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

Use inline-block to limit label width to content #14478

Merged
merged 3 commits into from
Aug 29, 2019

Conversation

sbardian
Copy link
Contributor

@sbardian sbardian commented Mar 17, 2019

Description

Closes #9612. Use display: inline-block instead of display: block to limit BaseControl label to width of the content of the label.

How has this been tested?

  • npm run test all tests successful.
  • manually looked at different blocks with labels. Obviously not an exhaustive test, and will be willing to look at anything else to confirm this change.

Screenshots

before:
image

after:
image

before:
image

after:
image

Types of changes

Bug fix (non-breaking change which fixes an issue)

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.

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 @sbardian thank you for your contribution.
I did a set of tests and I only found a small regression.
If you add a video block, you will verify the poster image button does not appear as expected.
image

I guess a possible solution is to add a display block to the poster image.

@sbardian sbardian force-pushed the try/basecontrol-label-style branch from 9a1db49 to 3fad50a Compare April 3, 2019 14:35
@sbardian
Copy link
Contributor Author

sbardian commented Apr 3, 2019

I have updated the poster control button to use display: block. This should only change the poster control button, but makes we wonder if others might need this as well. . . thoughts?

@sbardian
Copy link
Contributor Author

@jorgefilipecosta let me know if you would like other updates, or can see/think of other issues this might cause?

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 @sbardian, thank you for these changes it looks good to me.
I guess this change worths a note in the changelog https://github.com/WordPress/gutenberg//blob/6a26e3682ff79be3a031d283b6466a236f9748db/packages/components/CHANGELOG.md as an improvement under the last unreleased version.

@sbardian
Copy link
Contributor Author

@jorgefilipecosta I have the notes added. Thanks!

@gziolo gziolo added [Package] Components /packages/components Needs Design Feedback Needs general design feedback. [Type] Enhancement A suggestion for improvement. and removed Needs Design Feedback Needs general design feedback. labels Jun 20, 2019
@draganescu
Copy link
Contributor

draganescu commented Jul 29, 2019

Hi @sbardian I tried to visually test this but whenever I checkout the branch the build fails. Could you rebase it with latest master in your fork?

@sbardian sbardian force-pushed the try/basecontrol-label-style branch from cea2bc9 to 03c7ceb Compare July 29, 2019 17:18
@sbardian
Copy link
Contributor Author

Sorry for all the reviews on this. Only thing I can think of is history was rewritten in master at some point? Shouldn't have conflicts now.

Copy link
Member

@noisysocks noisysocks left a comment

Choose a reason for hiding this comment

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

Thanks for the fix, @sbardian!

@noisysocks noisysocks merged commit f9a342a into WordPress:master Aug 29, 2019
@gziolo gziolo added this to the Gutenberg 6.5 milestone Aug 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The BaseControl (TextControl etc.) label is too wide
5 participants