Unit Control: Always display current unit value if valid #34768
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
andrewserong
added
[Block] Columns
|
Size Change: -238 B (0%) Total Size: 1.06 MB
ℹ️ View Unchanged
|
andrewserong
requested review from
ajitbohra and
chrisvanpatten
as code owners
2 tasks
stacimc
reviewed
Sep 14, 2021
There was a problem hiding this comment.
This looks good to me! Definitely an interesting problem 🤔 To me this approach strikes a fairer balance between preventing broken UI and respecting the configuration in theme.json than enforcing 'required' units for the block.
I tested:
- In Twenty Twenty, '%' unit is selected when adding a 30/70 (or 25/50/25 etc) configuration
- With '%' still selected, value is treated as a percentage even if changed
- If the unit is changed, '%' is no longer available in the dropdown
- '%' is not available as an option for other configurations that don't set it by default (eg 50/50)
- '%' is always available when enabled in the theme.json
- Test coverage looks great 👍
ciampo
reviewed
Sep 15, 2021
| * | ||
| * @param {number|string} currentValue Selected value to parse. | ||
| * @param {string} legacyUnit Legacy unit value, if currentValue needs it appended. | ||
| * @param {Array<Object>} units List of available units. |
There was a problem hiding this comment.
Nit: It'd be great if we could leverage the @typedef and @property JSDocs tags to type the Units object better in this file (here's an example of how this syntax is used )
There was a problem hiding this comment.
I've added in a @typedef at the top of the file to create a definition of the unit object. Let me know if it needs further changes — I see there's a proposal to add in support for the keyword any for the step value in #34542. Since that isn't supported yet, I haven't added it to the type, but happy to add it in if you think we should get it in earlier.
andrewserong
force-pushed
the
fix/unit-control-always-display-current-unit-value
branch
from
eea67d9 to
f2d44a3
Compare
|
Thanks for the feedback @ciampo, it's very much appreciated! I've had a go at adding in the |
aristath
approved these changes
Sep 16, 2021
ciampo
reviewed
Sep 16, 2021
There was a problem hiding this comment.
Thank you for addressing the feedback!
Longer-term it'd be great for us to move this component to TypeScript, too — I'll write myself a note in case I get spare time at any point further down the track, and if no one beats me to it 🙂
I had a quick look at inspecting the code and I can already spot a few inconsistencies — mainly related to the fact that the unit is typed as string|number. A quick way to spot these is to change the extension of the file to .ts and re-run the ts-check
The conversion to TypeScript would be a welcome one, and seeing the bugs just listed above, probably should be treated with a higher priority than we initially estimated.
Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
|
Thanks again for the detailed notes @ciampo! I've updated the README, and great point about the inconsistencies when we switch the utils file to |
ciampo
approved these changes
Sep 17, 2021
There was a problem hiding this comment.
Thank you @andrewserong for working on this and addressing all feedback! Code changes LGTM and test well as per instructions 🚀
I'm happy to start taking a look at refactoring this component to TypeScript later on next week if it doesn't need to be done immediately, I'm keen to get the TS practice 🙂
That would be fantastic. Please keep me in the loop when you do, I'm definitely happy to help and review!
andrewserong
deleted the
fix/unit-control-always-display-current-unit-value
branch
7 tasks
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.
Description
Fixes #34096
In #34096 it was reported that in themes that don't include the
%unit in the theme.jsonsettings.spacing.unitslist, that percentage width column blocks display the current value aspxinstead of as%. This PR explores ensuring that if there is a current value, that its unit is displayed correctly in the UnitControl's drop-down.The idea here is that whether or not we wish to hide or disable a particular unit, if the current value has somehow managed to be a valid CSS unit, we should still do our best to display it correctly and allow it to be editable. Once the user switches to another unit, then the provided list of allowed units will take precedence.
For ease of testing, the TT1-Blocks theme is a good candidate, as it does not list the
%unit, and the Columns block is a good block to test, as its placeholder variations use percentage units.How has this been tested?
Test that % units are displayed correctly in TT1-Blocks
%%should workpx)%Test that the % unit doesn't show up where it isn't expected
%10%and hit ENTER. It should not let you commit the value as a percentage.Run tests
Screenshots
Before
Note that with TT1-Blocks active, after selecting the Columns block's 70 / 30 variation, the current value is displayed as if it's in
pxunits. (If you switch to the code view, you can see that the attribute's value is currently in%).After
After selecting the Columns block's 70 / 30 variation, the current value is correctly rendered with the
%unit. Switching away from that unit will hide the availability of the unit, as the theme is still attempting to keep it hidden. Here, this PR attempts to respect the settings, while still displaying an accurate value.Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.jsfiles for terms that need renaming or removal). (This change only applies to the web-based component)