LineHeightControl: Migrate internals to NumberControl (Part 2: Behavior)#37164
Merged
mirka merged 4 commits intoFeb 15, 2022
Merged
Conversation
10 tasks
|
Size Change: +112 B (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
mirka
force-pushed
the
migrate/line-height-control-styles
branch
from
94c0562 to
05b3631
Compare
mirka
force-pushed
the
migrate/line-height-control-behavior
branch
from
ab03dc8 to
f9d97a7
Compare
aaronrobertshaw
left a comment
aaronrobertshaw
left a comment
Contributor
There was a problem hiding this comment.
Nice work @mirka ✨
This tests pretty well for me across Chrome, Firefox, and Safari.
The only difference I could see between the migrated and legacy controls was the value type in some circumstances. For example, when using the keyboard to step the value up or down, the type in the migrated control stays as a number whereas the legacy control's value changes to a string.
Could this be a problem?
Kapture.2021-12-07.at.12.47.26.mp4
ciampo
reviewed
Dec 7, 2021
ciampo
left a comment
ciampo
left a comment
Contributor
There was a problem hiding this comment.
the type in the migrated control stays as a number whereas the legacy control's value changes to a string.
Could this be a problem?
I also noticed this inconsistency — although the legacy component also seems to have partially this behavior.
Legacy component:
- Value starts as
undefined - The first interaction (step up or down, mouse or keyboard) changes the value to the type
number - Any following interaction (step up or down, mouse or keyboard) changes the type to
string
New component:
- Value starts as
undefined - The first interaction (step up or down, mouse or keyboard) changes the value to the type
number - Any following mouse interaction (click on arrow up/down) changes the value to the type
string - Any following keyboard interaction (arrow up/down) changes the value to the type
number
I think we have 2 choices:
- adjust the new component to follow the (quirky) behaviour of the legacy component
- decide to always adopt the same type (
numberorstring) when the value is defined (I don't think this would be a breaking change, given how the legacy component already mixed number and string)
Just noting that LineHeightControl's README actually mentions that value can be both of type string and number.
Member
Author
|
Great testing, thank you!
^ This is the ideal outcome I think. Low priority, but it might come up naturally in the TypeScript refactor for NumberControl. |
mirka
force-pushed
the
migrate/line-height-control-styles
branch
from
7b0d262 to
9440108
Compare
mirka
force-pushed
the
migrate/line-height-control-behavior
branch
from
93c831c to
cce8db8
Compare
Member
Author
|
This PR has been rebased with the latest trunk and #37160. No changes in the behavioral code, but ready for a final check please 👀 |
ciampo
approved these changes
Feb 11, 2022
Contributor
There was a problem hiding this comment.
Code changes LGTM 🚀
Follow-ups (apart from the part 1 PR):
- Look into
composeStateReducers()and potentially update it to avoid state mutations (as discussed in https://github.com/WordPress/gutenberg/pull/37164/files#r764115916) (needs new tracking issue - Standardize the type accepted as an input (as discussed in #37164 (comment)) (I've updated #35744 to include this task during the refactor to TypeScript)
Member
Author
Thanks, I'll add an issue for this after merging to trunk. |
mirka
added a commit
that referenced
this pull request
Feb 16, 2022
* Add story for LineHeightControl * Keep legacy styling * Add deprecation warning * Update docs * Update changelog * Update style deprecation to match new guidelines * Update usage in Typography Panel * Update usage in Global Styles * Update docs * Tweak stories * Update story for ToolsPanel * Add deprecation note to prop readme * LineHeightControl: Migrate internals to `NumberControl` (Part 2: Behavior) (#37164) * Maintain previous behavior via state reducer * Add tests * Add comment about strange reducer behavior * Fix tests * Remove temporary code
8 tasks
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Will merge into #37160 (changelog will be updated there)
Prerequisite for #36196
Description
As part of changing the underlying component from
TextControltoNumberControl, we need to port some existing, line-height-specific logic. The logic handles what happens on the first interaction from an unset state.NumberControlSpecial handling is added for entering
0, because browser event-wise this can look very similar to a click on the down spin button:0All this logic was ported from the original (event-based, direct state manipulation) to fit the composed state reducer model of
InputControl.How has this been tested?
npm run storybook:devUI tests have been added for the stepping behavior. Unfortunately I don't think we can test the "Enter
0" case in jsdom, as the logic relies on some deeper event behavior. (These number input events differ a bit even across browsers, so it would require Playwright E2E for reliable testing. Probably not worth it here.)Types of changes
New feature (non-breaking change which adds functionality)
Checklist:
*.native.jsfiles for terms that need renaming or removal).