Components: migrate StyledLabel to SCSS module - #80001
Conversation
github-actions
Bot
added
[Package] Components
3a815c3 to
3d497b7
Compare
im3dabasia
added
the
[Type] Code Quality
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
ciampo
left a comment
ciampo
left a comment
There was a problem hiding this comment.
Thank you for working on this 🙏
On top of inline comments, some general feedback:
- looks like we should re-generate snapshots throughout the repo.
- we're missing a CHANGELOG entry
|
Hello @im3dabasia ! do you have capacity to continue the work on this PR? |
|
Yeah, I'll work on this PR and ping you by the end of the day once I've addressed all the feedback. |
|
Hi @ciampo, Hopefully 🤞 the CI will be happy now, and I've addressed all of your feedback as well. Apologies for the delay. Over to you! Could you please review the PR and let me know if there's anything else you'd like me to change? |
| text-transform: uppercase; | ||
| } | ||
|
|
||
| .help { |
There was a problem hiding this comment.
I believe we're missing text-wrap: pretty compared to the previous implementation?
|
|
||
| return ( | ||
| <Wrapper className={ className }> | ||
| <Wrapper className={ clsx( 'components-base-control', className ) }> |
There was a problem hiding this comment.
The regenerated snapshots now contain duplicated public classes, for example:
class="style-wrapper components-base-control components-base-control"
class="style-field components-base-control__field components-base-control__field"That is not trunk-equivalent. useContextSystem( props, 'BaseControl' ) already supplies components-base-control in className, and trunk already passed components-base-control__field from the BaseControl call site. We should avoid baking those same classes into both sides of the migration.
The narrower trunk-parity fix is:
- keep
<Wrapper className={ className }>inBaseControl - keep
<StyledField className="components-base-control__field">inBaseControl - remove
'components-base-control__field'fromUnforwardedStyledField
That should remove the duplicate classes from snapshots while preserving the public classes where they existed before.
| <PolymorphicElement | ||
| ref={ ref } | ||
| { ...props } | ||
| className={ clsx( |
There was a problem hiding this comment.
Same here re. duplicate class names
…label-to-css # Conflicts: # packages/components/src/toggle-group-control/test/__snapshots__/index.tsx.snap
2 participants
What?
Part of #66806
Migrates
StyledLabelaway from Emotion and onto an SCSS Module, while preserving the existingas="legend"behavior used byFocalPointPicker,BorderControl, and related consumers.Why?
StyledLabel is one of the representative styled components that relies on Emotion’s native as support without going through View. Migrating it helps validate the non-Emotion polymorphic pattern for shared label primitives and removes another piece of Emotion usage from @wordpress/components.
How?
Testing Instructions
Use of AI Tools
Used GitHub Copilot to help plan the migration, update the shared BaseControl style surface, and add focused test coverage. All generated changes were reviewed and validated manually.