Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Label Support for URLInput Component #15669

Merged
merged 17 commits into from
Nov 13, 2019

Conversation

pstonier
Copy link
Contributor

@pstonier pstonier commented May 15, 2019

Description

Added the ability to set a label for the URLInput field to follow the same structure as the TextControl.

How has this been tested?

Locally in Chrome.

Screenshots

Screen Shot 2019-05-15 at 3 29 22 PM

Screen Shot 2019-05-15 at 4 37 51 PM

Types of changes

  • Replaces the container div of the component with a BaseControl component to leverage the label support.
  • Updates HTML Structure to conditionally load a label if defined.

Checklist:

  • [ X ] My code is tested.
  • [ X ] My code follows the WordPress code style.
  • [ X ] My code follows the accessibility standards.
  • [X ] My code has proper inline documentation.
  • [ X] I've included developer documentation if appropriate.

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @pstonier, thank you for your contribution.
I used the following test component to verify differences between components with labels and components without label and to compare against the result in master:

	const [ url, setUrl ] = useState( '' );
	return (
		<>
			<URLInput
				value={ url }
				onChange={ setUrl }
			/>
			<URLInput
				label="With label"
				value={ url }
				onChange={ setUrl }
			/>
		</>
	);

Things worked as expected, and the only difference between the result in this branch and the result in the master is that in this branch the label appears.

I noticed that when this component is used in the inspector without additional styles changes, the dimensions are wrong, and the result is unexpected, but that is not a regression from these changes.

packages/block-editor/src/components/url-input/README.md Outdated Show resolved Hide resolved
packages/block-editor/src/components/url-input/index.js Outdated Show resolved Hide resolved
@jorgefilipecosta jorgefilipecosta added [Type] Enhancement A suggestion for improvement. [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). labels May 17, 2019
@pstonier
Copy link
Contributor Author

I agree with your suggestions and have submitted the changes.

@gziolo gziolo added the Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code label May 25, 2019
@talldan
Copy link
Contributor

talldan commented Jul 16, 2019

It looks like there's an issue with the styles in the link popover:

Screen Shot 2019-07-16 at 4 26 52 pm

There's some extra bottom margin added by BaseControl.

@pstonier
Copy link
Contributor Author

Thanks for catching that. The most recent commit should fix it.

.components-base-control:last-child,
.components-base-control:last-child .components-base-control__field {
margin-bottom: 0;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be necessary, but it didn't fix the styling problem for me. Adding the following rule, removed the extra space below the input:

.block-editor-url-input > .components-base-control__field {
	margin-bottom: 0;
}

@talldan is this the margin that you were referring to?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @pablinos. Yep, that looks like it.

@pento
Copy link
Member

pento commented Oct 15, 2019

I pushed a couple of commits to finish this PR off, and tested as a replacement for the hackery in Automattic/jetpack#13745.

@pstonier: Are you able to rebase this PR? GitHub was acting up on me when I tried to push a rebase, it should hopefully work for you.

@pento pento force-pushed the update/url-input-label branch from aae416a to e4aeb7e Compare October 16, 2019 01:42
pento and others added 3 commits November 13, 2019 12:42
Removes the defualt base control bottom margin when in a URLPopover.
This also removes its usage from the Button component.
@pento pento force-pushed the update/url-input-label branch from adc50a8 to dccfa7e Compare November 13, 2019 01:47
@pento pento added this to the Gutenberg 7.0 milestone Nov 13, 2019
Copy link
Member

@pento pento left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm liking where this is at. Thank you for your persistence in getting this across the line, @pstonier!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants