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

Update/base control component #18646

Merged
merged 4 commits into from Nov 25, 2019

Conversation

@fernandovbs
Copy link
Contributor

fernandovbs commented Nov 20, 2019

Description

This PR addresses a missing case in base-control specified in #18166. When id is not passed and hideLabelFromVision is passed in the label will still be visually displayed.

How has this been tested?

I've created a story in Storybook to confirm that the component are rendered correctly. I'll submit another PR for it.

Types of changes

Included a new condition to prevent the label from showing when hideLabelFromVision is true and no id is passed.

Checklist:

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

This comment has been minimized.

Copy link
Member

mkaz commented Nov 23, 2019

The change looks good. What do you think of using a ternary to make it a little cleaner to see what is happening? I'm open to suggestions it is a tricky set of code to format and look clean

{ label && id && ( hideLabelFromVision ?
	<VisuallyHidden
		as="label"
		htmlFor={ id }>{ label }
	</VisuallyHidden> :
	<label
		className="components-base-control__label"
		htmlFor={ id }>{ label }
	</label>
) }
{ label && ! id && ( hideLabelFromVision ?
	<VisuallyHidden
		as="label">{ label }
	</VisuallyHidden> :
	<BaseControl.VisualLabel>{ label }</BaseControl.VisualLabel>
) }
@fernandovbs

This comment has been minimized.

Copy link
Contributor Author

fernandovbs commented Nov 23, 2019

Hello, @mkaz it's the first time that I contribute, so I tried to make the smallest possible change.
For sure the ternary looks much better.
I'll update the code. Thank's for the feedback 😄

@fernandovbs fernandovbs force-pushed the fernandovbs:update/base-control-component branch from 8b2aea6 to 074d363 Nov 23, 2019
@mkaz

This comment has been minimized.

Copy link
Member

mkaz commented Nov 24, 2019

Thanks for the contribution, can you make one more small change, and its my fault because of the example I showed. Can you move the closing tag to the same line next to the { label } ? I don't want the markup to add an extra new line or space.

There are three spots, thanks!

{ label && id && ( hideLabelFromVision ?
	<VisuallyHidden
		as="label"
		htmlFor={ id }>{ label }</VisuallyHidden> :
	<label
		className="components-base-control__label"
		htmlFor={ id }>{ label }</label>
) }
{ label && ! id && ( hideLabelFromVision ?
	<VisuallyHidden
		as="label">{ label }</VisuallyHidden> :
	<BaseControl.VisualLabel>{ label }</BaseControl.VisualLabel>
) }
@fernandovbs

This comment has been minimized.

Copy link
Contributor Author

fernandovbs commented Nov 24, 2019

No problem @mkaz. Fixed.

@mkaz
mkaz approved these changes Nov 25, 2019
Copy link
Member

mkaz left a comment

Looks good, thanks for the contribution 👍

@mkaz mkaz merged commit 69e7fc7 into WordPress:master Nov 25, 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
Projects
None yet
2 participants
You can’t perform that action at this time.