[Components - ToggleGroupControlOption]: Calculate width from button content and remove LabelPlaceholderView
#39345
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
… content and remove `LabelPlaceholderView`
ntsekouras
added
[Type] Code Quality
ntsekouras
requested review from
mirka,
ciampo,
jasmussen,
ramonjd and
andrewserong
ntsekouras
changed the title
LabelPlaceholderView LabelPlaceholderView
|
Size Change: -53 B (0%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
ciampo
approved these changes
Mar 10, 2022
There was a problem hiding this comment.
So to sum up I think LabelPlaceholderView is completely obsolete - but feel free to correct me if I'm missing something. 😄
It does look like it. It'd be interesting to see when/why this was introduced, in order to understand this strange layout pattern.
The only difference that I can see is that each ToggleGroupOption now looks slightly narrower:
toggle-group-control-diff.mp4
This is probably caused by the fact that the placeholder view (which was deleted in this PR) had font-weight: bold; among its styles, which consequently made its text take more horizontal space — although I don't see it as an issue.
|
Thanks for the reviews!
I think this was just code written quite a long time ago, as part of the initial creation of g2 components and was ported this way. Similar case would be the aspectRatio component that it turned out we didn't need at all right now.
Right! If we want to tweak some styles there we should do it in the visible label and actually I think we discuss this in the
💯 |
|
I cherry-picked this PR into Gutenberg 12.8 release to prevent issues with the merge conflicts in the CHANGELOG file while publishing to npm with the automated script. |
gziolo
pushed a commit
that referenced
this pull request
Mar 11, 2022
…content and remove `LabelPlaceholderView` (#39345) * [Components - ToggleGroupControlOption]: Calculate widths from button content and remove `LabelPlaceholderView` * update changelog * fix tests
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.
What?
I made a POC for adding
iconsupport inToggleGroupControlcomponent and saw that we calculate the width of the labels in a weird and not proper way IMO. What we are doing now is having css for the main button that positionsabsolutethe element and centers it with atranslaterule, and we useLabelPlaceholderViewwhich is not visible(visibility: hidden;) but still in the document label to calculate the width. This seems wrong to me because:LabelPlaceholderView. If we wanted to change that - like icons support - we cannot do that.LabelPlaceholderViewwe also have aria-hidden which means the element is not exposed to an accessibility API, so it seems that it was just there for width calculation to me..So to sum up I think
LabelPlaceholderViewis completely obsolete - but feel free to correct me if I'm missing something. 😄Why?
Code quality change that will also enable us to add
iconsupportTesting Instructions
FontSizePickeror inPostFeaturedImagewhen you set aheight, etc... Also check the stories in storybook.