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

FontSizePicker: Make control take up full width #44559

Merged
merged 2 commits into from
Sep 29, 2022

Conversation

noisysocks
Copy link
Member

@noisysocks noisysocks commented Sep 29, 2022

What?

Removes the 280px max-width from FontSizePicker so that it takes up the full width of its parent container.

The Reset button will now also be 40px high when __size is set to __unstable-large.

Neither of these changes make any practical difference to Gutenberg which only uses FontSizePicker in the (narrow) sidebar and always hides the Reset button.

Why?

  • More consistent with most other components in @wordpress/components which take up the full width.
  • Styling such as this is the responsibility of the caller. @wordpress/components shouldn't know what a sidebar is or how wide it is.
  • 30px tall Reset button looks weird.

How?

Testing Instructions

Storybook:

  1. npm run storybook:dev
  2. Go to FontSizePicker
  3. Toggle on Set custom size

Gutenberg:

  1. npm run dev
  2. Global Styles → Typography → Text
  3. Check that the Size control looks the same as it does in trunk

Removes the 280px max-width from FontSizePicker so as to be more
consistent with other components. Styling such as this is the
responsibility of the caller.
@noisysocks noisysocks added [Type] Code Quality Issues or PRs that relate to code quality [Package] Components /packages/components labels Sep 29, 2022
@noisysocks noisysocks added [Type] Enhancement A suggestion for improvement. and removed [Type] Code Quality Issues or PRs that relate to code quality labels Sep 29, 2022
@noisysocks
Copy link
Member Author

noisysocks commented Sep 29, 2022

To me this is a bug but I classified it as an enhancement so that the minor version of @wordpress/components is bumped. I don't think it's a significant enough of a change to warrant a major version bump, but nor is it something that I'd want to slip in via a patch bump. I'm not really sure though so let me know how you'd approach this.

@noisysocks noisysocks added the [Type] Code Quality Issues or PRs that relate to code quality label Sep 29, 2022
@ciampo ciampo added this to In progress (owned) ⏳ in WordPress Components via automation Sep 29, 2022
@ciampo ciampo moved this from In progress (owned) ⏳ to In progress ⏳ (un-owned) in WordPress Components Sep 29, 2022
Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Tests well in Storybook (controls now go full width) and in the editor (no visual changes)

(tagging @mirka for visibility on the size change for the reset button)

More consistent with most other components in @wordpress/components which take up the full width.
Styling such as this is the responsibility of the caller. @wordpress/components shouldn't know what a sidebar is or how wide it is.

100% agree with these principles.

On a similar note, we have some work to do to remove the __experimentalIsRenderedInSidebar prop (as part of #42994) that was sneaked in as a temporary fix but has never been removed since 😅 Let me know if you want to have some fun with that too!

To me this is a bug but I classified it as an enhancement so that the minor version of @wordpress/components is bumped. I don't think it's a significant enough of a change to warrant a major version bump, but nor is it something that I'd want to slip in via a patch bump. I'm not really sure though so let me know how you'd approach this.

I'm not sure about this, probably @gziolo could share some wisdom here :)

@mirka
Copy link
Member

mirka commented Oct 12, 2022

Just for context, we have been classifying changes like this (removing a width constraint) as a substantial enough change to warrant a formal deprecation (e.g. #43230).

For this relatively higher-level component though, there is a much lower chance of consumers using it in arbitrary layouts, so it might be ok.

@ciampo
Copy link
Contributor

ciampo commented Oct 12, 2022

Just for context, we have been classifying changes like this (removing a width constraint) as a substantial enough change to warrant a formal deprecation (e.g. #43230).

For this relatively higher-level component though, there is a much lower chance of consumers using it in arbitrary layouts, so it might be ok.

That is my bad for not flagging this as warranting a formal deprecation — sorry!

As you said, this should be ok 🤞 but I'm also fine in case we feel that putting this change under a temporary __nextUnconstrainedWidth prop is the right thing to do here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Code Quality Issues or PRs that relate to code quality [Type] Enhancement A suggestion for improvement.
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants