close
The Wayback Machine - https://web.archive.org/web/20191214053422/https://github.com/WordPress/gutenberg/pull/15669
Skip to content

/gutenberg

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
Merged

Add Label Support for URLInput Component #15669

merged 17 commits into from Nov 13, 2019

Conversation

@pstonier
Copy link
Contributor

pstonier commented May 15, 2019 •
edited

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.
@pstonier pstonier requested review from aduth, ajitbohra, nerrad and ntwb as code owners May 16, 2019
@jorgefilipecosta
jorgefilipecosta reviewed May 17, 2019
View changes
Copy link
Member

jorgefilipecosta left a comment

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
@pstonier

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

pstonier commented May 17, 2019

I agree with your suggestions and have submitted the changes.
@talldan

This comment has been minimized.

Sign in to view
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

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

pstonier commented Jul 17, 2019

Thanks for catching that. The most recent commit should fix it.
@pablinos
pablinos reviewed Aug 18, 2019
View changes
packages/block-editor/src/components/url-input/style.scss Outdated
.components-base-control:last-child,
.components-base-control:last-child .components-base-control__field {
margin-bottom: 0;
}

This comment has been minimized.

Sign in to view
Copy link
@pablinos

pablinos Aug 18, 2019

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?

This comment has been minimized.

Sign in to view
Copy link
@talldan

talldan Aug 20, 2019

Contributor

Hey @pablinos. Yep, that looks like it.
@pento pento mentioned this pull request Oct 15, 2019
Merged
@pstonier pstonier requested a review from Soean as a code owner Oct 15, 2019
@pento

This comment has been minimized.

Sign in to view
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 pstonier:update/url-input-label branch from aae416a to e4aeb7e Oct 16, 2019
Paul Stonier added 9 commits May 15, 2019
Paul Stonier @pento
Add Label Support for URLInput Component
8e13521
Paul Stonier @pento
Added documentation: new props for label support
6edb065
Paul Stonier @pento
Links test updated to correlate to new HTML structure
2684711
Paul Stonier @pento
Removed inspector from id and id from documentation
150de1b
Paul Stonier @pento
fix confliciting const of id
6d67e40
Paul Stonier @pento
Fix duplicate const of ID with new baseControlID
34e2cc8
Paul Stonier @pento
fixed base control margin on URLPopover
0365094
Paul Stonier @pento
removed base control margin from URLInputButton modal
83a1b57
Paul Stonier @pento
remove pseudoclass of last-child from URLInputButton style
6b4fa5c
pento and others added 3 commits Oct 15, 2019
@pento
Tweak the styling
15038cb 
Removes the defualt base control bottom margin when in a URLPopover.
@pento
Remove the id attribute from URLInput
7eef574 
This also removes its usage from the Button component.
@pstonier @pento
fixed label undefined error
Loading status checks…
dccfa7e
@pento pento force-pushed the pstonier:update/url-input-label branch from adc50a8 to dccfa7e Nov 13, 2019
pento added 5 commits Nov 13, 2019
@pento
Re-apply dccfa7e?
Loading status checks…
691eb69
@pento
Revert changes to package-lock.json
Loading status checks…
39ee8a2
@pento
Update the LinkControl snapshot.
Loading status checks…
12dbbd9
@pento
Remove an unused var.
9450353
@pento
Update the changelog.
Loading status checks…
e97c9df
@pento pento added this to the Gutenberg 7.0 milestone Nov 13, 2019
@pento
pento approved these changes Nov 13, 2019
View changes
Copy link
Member

pento left a comment

I'm liking where this is at. Thank you for your persistence in getting this across the line, @pstonier!
@pento pento merged commit 45a566c into WordPress:master Nov 13, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pablinos pablinos

@jorgefilipecosta jorgefilipecosta

@pento pento

@youknowriad youknowriad

@noisysocks noisysocks

@talldan talldan

@Soean Soean

@gziolo gziolo

@ntwb ntwb

@ajitbohra ajitbohra

@nerrad nerrad

@aduth aduth

@ellatrix ellatrix

Assignees
No one assigned
Labels
Accessibility (a11y) Good First Review [Type] Enhancement
Projects
None yet
Milestone
  Gutenberg 7.0
6 participants
@pstonier @talldan @pento @pablinos @jorgefilipecosta @gziolo
You can’t perform that action at this time.