Font Library modal: Try to improve checkbox labelling #58339
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
mikachan
added
[Type] Enhancement
|
Size Change: -70 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 9f2026b. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7702978643
|
mirka
reviewed
Jan 30, 2024
There was a problem hiding this comment.
Thanks for the follow-up! The revert to the CheckboxControl API changes look good.
On the Font Library side of things, the approach feels a little bit roundabout to me, especially with the faux button we're trying to do in FontFaceDemo. I just proposed adding a custom label example to the Storybook in #58438 that I think is the most straightforward approach in most cases, similar to one of the alternative methods you suggested in the PR description. Using a <label> element would avoid having to implement a faux button, and since an <img> is phrasing content, I think it should be permitted to include inside a label node.
What do you think?
|
Thanks for taking a look, @mirka!
I agree, although I tried the quickest/most simple solution first 😅 This is also why I attempted my original approach in #58299, as I'd rather take advantage of all functionality already available using a native Thanks so much for the example in #58438! Although, does this not render two |
Good point, I was still referring to the implementation in trunk. I think the conditional rendering logic you added in #56158 was great — that change was basically a bug fix, since rendering an empty label is nonsensical. It's just the type changes that were unnecessary (i.e. adding a |
|
One of the main pieces of feedback received on #56158 was that we were allowing consumers of With the latest suggested changes, users would still be able to do so by omitting the A few ideas that came to mind:
In any case, we should make sure that consumers of |
|
For the scope of this PR, I suggest limiting the CheckboxControl changes to reverting the addition of a Anything else is enhancement territory, which should be discussed separately IMO 🙂 |
|
Thanks so much, both! I agree with the idea not to increase the scope of this PR too much, so I've gone ahead and added the conditional rendering of the label tag in And then I've updated the Font Library checkbox markup in line with your example above, so there is now a separate label tag that holds the preview image or text. Committed here: 239d4f4 This tests well for me and I feel much happier with this approach. Thank you for helping me work through it 🙇 |
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?
This PR is an attempt to address #58299, and covers the following:
CheckboxControlcomponent added in Font Library: Improve usability of font variant selection #56158CheckboxControlcomponentWhy?
To improve accessibility and usability of these checkboxes whilst maintaining the functionality of the
CheckboxControlcomponent.How?
The main challenge with the current implementation of the Font Library checkboxes is that the visible label can either be text or an image, depending on if the font face provides a preview image (e.g. an SVG). This logic cannot currently be passed to the
CheckboxControlcomponent, as thelabelprop only accepts a string (which I agree with).Also, the older design for the font library checkboxes had the checkboxes to the right of the label, rather than the left. Now the design has been updated to be in line with the
CheckboxControlcomponent, where the checkbox is to the left of the label. This makes it much easier to use the existing functionality of this component.The solution in this PR is to provide a label in the
CheckboxControl, and then hide the label element that the component creates using CSS. The CSS should visibly hide the label while still allowing it to be accessed by screen-readers. This has worked well in my testing so far.There has also been an
onClickhandler added to theFontFaceDemocomponent, which allows the checkbox to be toggled when this component is clicked. Otherwise, only the checkbox itself can be interacted with to toggle the checkbox value, and I don't think this is good for accessibility or usability.This is one attempt at an alternative solution to handling these checkbox labels, and I'd love feedback on whether this is a good approach or if we should try alternatives.
Possible other alternatives:
aria-labelledbyattribute to theCheckboxControl, and reference theFontFaceDemoelement as the labelCheckboxControlcomponent (that doesn't allow the label to befalse)Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast
Screen.Recording.2024-01-26.at.21.51.32.mov