Update control labels to the new uppercase styles #42789
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mirka
added
[Package] Components
|
Size Change: -66 B (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
mirka
commented
Jul 28, 2022
Comment on lines
+63
to
64
| // @ts-expect-error - Unclear how to fix, see also https://github.com/WordPress/gutenberg/pull/39468#discussion_r827150516 | ||
| BaseControl.VisualLabel.displayName = 'BaseControl.VisualLabel'; |
There was a problem hiding this comment.
Side effect of the changing the TS types for VisualLabel! We might want to look into a solution if we encounter this more.
Comment on lines
40
to
44
| /** | ||
| * Removes Chrome/Safari/Firefox user agent stylesheet padding from | ||
| * StyledLabel when it is rendered as a legend. | ||
| */ | ||
| padding: 0; |
There was a problem hiding this comment.
This legend reset has been moved from being included only in StyledLabel, to being shared between both StyledLabel and StyledVisualLabel . This is because StyledVisualLabel (a.k.a. BaseControl.VisualLabel) is also likely to be used as="legend".
There was a problem hiding this comment.
Nice work as always @mirka 👍
In general, this PR tested well for me. I only encountered a few minor issues.
- The passing of props to the
BaseControl.VisualLabelwhich you've already flagged. After applying a quick fix for that, legends were rendered correctly. - The capitalization of the labels meant the Letter Spacing control in the editor's typography panel was truncated. I believe the root of this issue was the Font Appearance control forcing the panel's first column to be greater than 50%, thus restricting the space for the letter spacing label. I've put up a quick fix for that separately in #42795
- There are a few unit tests that may need snapshots updated after these changes.
Once tweaks fixing 1 & 3 are in place, I'll happily approve this PR. Thanks for wrangling all of this. 🙇
ciampo
reviewed
Jul 29, 2022
There was a problem hiding this comment.
👏 A much needed improvement, thank you for working on it!
We should streamline this more, but there are also some TypeScript complications so I'm not going to do it in this PR.
....
- InputControl: Fix
LabelWrapperline-height inconsistency #42788- (maybe worth looking into) Tweak LabelWrapper's max-width. #34609
- The
ControlLabel/FormGroupcomponents are left untouched because they aren't publicly exported or used anywhere.- InputControl: The
sizeprop is being incorrectly passed through to aText-based label that results in an invalidfont-sizevalue. This isn't making a visual difference because the value is overridden anyway, but I'll fix it separately in InputControl: Fix incorrect size prop passing toText#42793.
Looking forward to us being able to iron out also these inconsistencies, as I believe that can greatly improve the overall look & feel of the library
mirka
force-pushed
the
update/control-labels
branch
from
27cd670
to
84f5232
Compare
mirka
force-pushed
the
update/control-labels
branch
from
84f5232
to
486f9a3
Compare
2 tasks
aaronrobertshaw
approved these changes
Aug 5, 2022
There was a problem hiding this comment.
Thanks for continuing to refine this one @mirka 👍
All the issues I encountered in my initial review have been resolved (#42795 withstanding). It appears that the other feedback received has also been addressed.
I left one other minor question but am happy to give this a green light from me. Nice work!
| @@ -120,12 +120,9 @@ export default function BoxControl( { | |||
| <Root id={ id } role="group" aria-labelledby={ headingId }> | |||
| <Header className="component-box-control__header"> | |||
| <FlexItem> | |||
| <Text | |||
| id={ headingId } | |||
| className="component-box-control__label" | |||
There was a problem hiding this comment.
I take it we are ok removing the components-box-control__label class here given the BoxControl is still exported as experimental?
I ask, as later in this PR, we keep the components-form-token-field__label class desite removing the styles for it.
There was a problem hiding this comment.
Great catch! I am actually quite inclined to remove unused CSS classes even if they are in stable components, unless it's a class that we can imagine a lot of third-party consumers to be overriding in a crucial way (e.g. components-popover__content). (@ciampo Does that sound about right or should we be more conservative about it?)
The reason I left components-form-token-field__label intact is more because it's being used as a selector in an old e2e test 🙃
There was a problem hiding this comment.
Sorry for the late answer, I'm currently a bit behind on my review queue 😅
I believe we should address this situation in a more general way (and potentially even add a paragraph about the strategy we decide to adopt to the contributors' guidelines).
In general. I'd love to remove hardcoded classnames — in practice, it may cause breakage for consumers that rely on them.
Should we consider the removal of hardcoded classnames a breaking change?
- On one hand, hardcoded classnames are not part of the public APIs of the components (except for a these 4 components 🤦 ) — and so, technically it would not be a breaking change
- On the other hand, hardcoded classnames have been used outside of the package for years (both inside and outside the Gutenberg repo).
For example, even in the case of the removal component-box-control__label, we're not 100% guaranteed that it won't cause breakage to some external consumers.
In general, I believe we should at least aim at removing all instances of component-* classnames outside of the components' package. After that, depending on how conservative we want to be, we could:
- also delete the hardcoded classnames in the source components
- keep them as "legacy" support, but add a ESLINT rule forbidding their usage in Gutenberg (although this won't stop external consumers from using them)
What do folks think?
There was a problem hiding this comment.
I believe we should address this situation in a more general way (and potentially even add a paragraph about the strategy we decide to adopt to the contributors' guidelines).
Absolutely, I don't think it's explicitly written anywhere.
Should we consider the removal of hardcoded classnames a breaking change?
Could it cause breakage? Yes. Should we consider it a Breaking Change? No.
I was a consumer of the wp-components package before coming to the maintainer side, and I was never under the impression that CSS classnames (or styling, or DOM structure, for that matter) were guaranteed not to change. I think most people realize that overriding internal styling is fragile, and that it's their responsibility to protect against breakage if they are going to do so. Unless these classnames are publicly documented, as in the examples you raised 😅
There was a problem hiding this comment.
Personally, I agree with the notion that while it might cause breakage, we shouldn't necessarily consider it a breaking change. That said, I think communication and documentation will be key to maintaining as much good will as possible.
There was a problem hiding this comment.
Agree with both of you. I opened #43083 to keep track of this task separately
There was a problem hiding this comment.
For reference, the project does have existing documentation about backwards compatibility around class names and DOM updates.
Class names and DOM nodes used inside the tree of React components are not considered part of the public API and can be modified.
Changes to these should be done with caution as it can affect the styling and behavior of third-party code (Even if they should not rely on these in the first place). Keep the old ones if possible. If not, document the changes and write a dev note.
There was a problem hiding this comment.
Thank you @nerrad ! Seems to align with our thoughts above
3 tasks
This was referenced Oct 7, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #42782
What?
Updates all control component labels to the new uppercase styles.
Why?
Most of the missing font-size declarations discovered in #42781 were in control labels, so this is a good opportunity to do a sweep and update.
How?
The label styles in our repo currently originate from either
InputControlorBaseControl. Although we should eventually merge them completely, as an incremental step in this PR I've extracted the shared typography styles between the two into a low-level mixin calledbaseLabelTypography.Next, all components that have ad hoc styled labels have been replaced with BaseControl's
StyledLabelorBaseControl.VisualLabel. The two are basically the same, except thatStyledLabelis<label>by default and is not publicly exported, whileBaseControl.VisualLabelis<span>by default and publicly exported. Both are polymorphic. We should streamline this more, but there are also some TypeScript complications so I'm not going to do it in this PR.Testing Instructions
npm run storybook:devand check all "control" components.npm run devand check the components in the post/site editors.Out of scope for this PR
LabelWrapperline-height inconsistency #42788ControlLabel/FormGroupcomponents are left untouched because they aren't publicly exported or used anywhere.sizeprop is being incorrectly passed through to aText-based label that results in an invalidfont-sizevalue. This isn't making a visual difference because the value is overridden anyway, but I'll fix it separately in InputControl: Fix incorrect size prop passing toText#42793.Screenshots or screencast
CleanShot.2022-07-29.at.04.49.17.mp4