-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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 control labels to the new uppercase styles #42789
Conversation
Size Change: -66 B (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
// @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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side effect of the changing the TS types for VisualLabel! We might want to look into a solution if we encounter this more.
/** | ||
* 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.VisualLabel
which 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. 🙇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏 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
LabelWrapper
line-height inconsistency #42788- (maybe worth looking into) Tweak LabelWrapper's max-width. #34609
- The
ControlLabel
/FormGroup
components are left untouched because they aren't publicly exported or used anywhere.- InputControl: The
size
prop is being incorrectly passed through to aText
-based label that results in an invalidfont-size
value. 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
27cd670
to
84f5232
Compare
84f5232
to
486f9a3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with both of you. I opened #43083 to keep track of this task separately
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @nerrad ! Seems to align with our thoughts above
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
InputControl
orBaseControl
. 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
StyledLabel
orBaseControl.VisualLabel
. The two are basically the same, except thatStyledLabel
is<label>
by default and is not publicly exported, whileBaseControl.VisualLabel
is<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:dev
and check all "control" components.npm run dev
and check the components in the post/site editors.Out of scope for this PR
LabelWrapper
line-height inconsistency #42788ControlLabel
/FormGroup
components are left untouched because they aren't publicly exported or used anywhere.size
prop is being incorrectly passed through to aText
-based label that results in an invalidfont-size
value. 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