LineHeightControl: Enhance interactions by migrating internals to NumberControl on Style options.
#37160
Conversation
|
Size Change: +2.12 kB (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
mirka
force-pushed
the
migrate/line-height-control-styles
branch
from
94c0562
to
05b3631
Compare
There was a problem hiding this comment.
Thanks for working on this @mirka 👍
I appreciate the effort placing the legacy control alongside the migrated one in the Storybook. Made testing much easier.
They should look identical.
To the naked eye, the controls look the same. Upon inspecting the controls in dev tools I see there is the slightest of discrepancies between them.
In Chrome and Firefox it is only a fraction of a pixel so you could ignore it. Safari appears to have a 3px difference in height between the migrated and legacy controls.
| Chrome / Firefox | Safari |
|---|---|
 |
 |
For the record, the migrated control has a different line-height on the label and some weird spacing styles mixing padding and margin. The slight discrepancy can be ironed out with the following tweaks applied to the migrated control's label.
line-height: normal;
padding-bottom: 0;
margin-bottom: 4px;
| Migrated - Before | Legacy | Migrated - After |
|---|---|---|
 |
 |
 |
I don't feel strongly about whether this needs attention, I'll leave that up to you.
There was a problem hiding this comment.
These all affect layout for existing external usage, and I'm assuming we'll need to keep supporting them by default. However, we do not want any of these legacy styles in our internal usage of the component (i.e. in Typography panel).
In this PR, I am proposing a
__unstableHasLegacyStylesprop
I'd like to think about how we can possibly switch to the new styles in the future (and potentially remove support the legacy styles) — __unstable* props are definitely useful to help us introduce this kind of changes, but I'd like to have plan for them to not stick around, and therefore either becoming "stable" or going away.
In this specific case, I wonder if we could:
- add, in this PR, a warning when
__unstableHasLegacyStyles === true, mentioning that from a future version of WordPress (eg 5.10) those legacy styles will be removed, and therefore all instances ofLineHeightControlwill be forced to use the new styles. Consumers of this component will be advised to set__unstableHasLegacyStylestofalseand adjust styles accordingly before that happens. - when the future version of WordPress mentioned above (e.g. 5.10) gets released, we can remove the
__unstableHasLegacyStylesprop from the code and force the new styles on theLineHeightControlcomponent
In Chrome and Firefox it is only a fraction of a pixel so you could ignore it. Safari appears to have a 3px difference in height between the migrated and legacy controls.
Aaron is correct about the discrepancy in Safari. I applied the suggested styles this way:
.block-editor-line-height-control {
&.has-legacy-styles {
margin-bottom: 24px;
// TODO: remove hardcoded classname
.components-input-control__label {
line-height: normal;
padding-bottom: 0;
margin-bottom: 4px;
}
// TODO: remove hardcoded classname
.components-input-control__container {
margin-top: 4px;
}
}
}This seems to fix Safari, but re-introduces a difference in Chrome. Those differences seem to be related to differences in the User Agent stylesheets (line height of the label and margin around the input element), and are not easy to iron out across all browsers while also making sure that the new and the legacy version look exactly the same. If we had to compromise and keep some discrepancies in one browser, probably Safari would be my pick given the lower usage.
Finally, a thought — should we look into standardising how *Control components are implemented internally, so that they all build on top of the same base control component? This would automatically standardise line-heights / input margins / etc and solve the problem at the root.
packages/block-editor/src/components/line-height-control/index.js
Outdated
Show resolved
Hide resolved
|
Good catch with the browser discrepancies, thank you! Speaking of, I recently got a high resolution setup, and it’s now clearly showing me that in Chrome the migrated
Agreed. Also it’d be unwise to continue using UA-dependent values like line-height In a new project I would reach for normalize.css but I guess that ship has sailed. We can expect to surface more cross-browser issues as we continue these component migrations. I think we’ll have to live with the fact that we can’t always replicate the legacy styling perfectly where there were underlying UA inconsistencies to begin with.
Yes, it was my understanding that we are headed toward that direction.
Great point, I added a deprecation warning. |
There was a problem hiding this comment.
it’d be unwise to continue using UA-dependent values like line-height normal — it perpetuates the problem.
💯
In a new project I would reach for normalize.css but I guess that ship has sailed.
I'd love to do so. We could look into it, but I'm afraid that doing so could introduce many regressions.
We can expect to surface more cross-browser issues as we continue these component migrations. I think we’ll have to live with the fact that we can’t always replicate the legacy styling perfectly where there were underlying UA inconsistencies to begin with.
That makes sense. I guess that the best compromise for this PR is to keep the styles as they currently are, and aim at solving the browser discrepancies by refactoring every *Control component to use internally BaseControl and its label.
--
We should probably rebase this PR before giving it a final look and focus on #37164
packages/block-editor/src/components/line-height-control/style.scss
Outdated
Show resolved
Hide resolved
mirka
force-pushed
the
migrate/line-height-control-styles
branch
from
7b0d262
to
9440108
Compare
|
I’ve updated this PR (added commits) to follow the style deprecation strategy discussed in #38540. The styling being deprecated is now only the
Existing usage in the Global Styles and block editor Typography Panels have been updated as well. |
| const deprecatedStyles = __nextHasNoMarginBottom | ||
| ? undefined | ||
| : { marginBottom: 24 }; |
There was a problem hiding this comment.
I hope this is simple and ephemeral enough to put in here as an inline style. The @wordpress/block-editor package itself doesn't really use Emotion yet, and we can avoid a hardcoded classname here if we just do it inline.
mirka
added
the
Needs Dev Note
There was a problem hiding this comment.
Thanks for your continued efforts on this 👍
I've given this other test across browsers, the storybook, and in the block editor.
✅ Legacy and updated components look and behave similarly in the Storybook
✅ Legacy and updated components look the same across browsers
(Fine in Chrome/FF, close enough in Safari)
✅ The block editor's typography panel layout and appearance has been maintained
❓ The typography panel storybook has new spacing added to it
As noted previously they are some differences between the legacy component and the updated version when viewed in Safari. I agree with Marco that if we must have a discrepancy, then it is best to be in Safari.
I've also left a couple of other minor comments but this should be pretty close.
packages/block-editor/src/components/line-height-control/README.md
Outdated
Show resolved
Hide resolved
…vior) (#37164) * Maintain previous behavior via state reducer * Add tests * Add comment about strange reducer behavior * Fix tests
cbravobernal
changed the title
NumberControl (Part 1: Styles)NumberControl on Style options.
cbravobernal
added
[Package] Block editor
Closes #37110
Prerequisite for #36196
Related to #36387
Description
Replaces the underlying component of
LineHeightControlfrom the olderTextControlto the newNumberControl. This will help us avoid adding hacky styles to achieve consistency with newer components.Previous description
First off, we need to decide how and how much we want to keep the legacy styling. Specifically, the style differences we have are:
These all affect layout for existing external usage, and I'm assuming we'll need to keep supporting them by default. However, we do not want any of these legacy styles in our internal usage of the component (i.e. in Typography panel).
In this PR, I am proposing a
__unstableHasLegacyStylesprop that can be used to remove these back-compat styles:This way, we can cleanly use the next-gen styles where we want to. Thoughts?
In accordance with the style deprecation strategy discussed in #38540, the outer margins have been deprecated and early adopters can opt-in via the
__nextHasNoMarginBottomprop. The stylesheet can now be completely removed, opening up a path to remove the.block-editor-line-height-controlhardcoded classname if desired.Existing usage in the Global Styles and block editor Typography Panels have also been updated, which lets us remove an internal style hack 🎉
TODO before merge
How has this been tested?
npm run storybook:dev(The Legacy component is there for testing purposes — it will be entirely deleted before merge.)
Screenshots
Types of changes
New feature (non-breaking change which adds functionality)
Checklist:
*.native.jsfiles for terms that need renaming or removal).