Merged
Conversation
|
Size Change: +68 B (0%) Total Size: 1.1 MB
ℹ️ View Unchanged
|
874c1ec to
96e5002
Compare
ramonjd
commented
Nov 22, 2021
packages/components/src/toggle-group-control/toggle-group-control-option/component.tsx
Outdated
Show resolved
Hide resolved
Member
|
Cool, thanks for looking into this! |
mtias
reviewed
Nov 22, 2021
ciampo
reviewed
Nov 23, 2021
Contributor
ciampo
left a comment
There was a problem hiding this comment.
Testing with Storybook, I noticed that the tooltip appears only when using the keyboard — is this by design?
toggle-tooltip.mp4
I'm not sure if wrapping a ToggleGroupControlOption in a Tooltip affects the semantics / accessibility of the component. From a quick look everything seems ok, but it would be great if @diegohaz would be able to offer any further insights.
Also, these changes on ToggleGroupControl should be reflected in the README and in the @wordpress/components package CHANGELOG.
Finally, some time ago we had introduced a new Flyout component which was supposed to replace Popover in the long run, but I'm not sure if we should be using it. Probably another point where @diegohaz can give some advice.
packages/components/src/toggle-group-control/toggle-group-control-option/component.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/toggle-group-control/toggle-group-control-option/component.tsx
Outdated
Show resolved
Hide resolved
Member
Author
|
Thanks for testing, everyone! 🙇♂️
The tooltip appears onMouseover, and with a bit of delay. I double-checked in FF, Safari and Chrome and it behaves as expected for me. Let me know if the problem persists on your side though and I can test further.
Ah got it! Thanks for the reminder! |
96e5002 to
761dacf
Compare
Member
Author
|
I've updated the prop, tests, readme and changelog in 761dacf5a0aa8598bc3138d71dd3de3b5e299025 Thanks for the feedback again! |
Contributor
aaronrobertshaw
left a comment
There was a problem hiding this comment.
This is looking pretty good so far @ramonjd, nice work ✨
✅ Unit tests pass
✅ Existing uses of the ToggleGroupControl in the editor still work
✅ Tooltips display when hoving with mouse or selecting via keyboard
❓ Storybook tooltip examples do not work (see comments below)
When switching to the showTooltip approach it appears as though the component's stories were missed.
packages/components/src/toggle-group-control/toggle-group-control-option/component.tsx
Outdated
Show resolved
Hide resolved
Member
Author
|
Thanks for leaving the suggestions @aaronrobertshaw I managed to commit all this from my phone thanks to you ☎️ |
9d484e9 to
9014929
Compare
ramonjd
commented
Nov 24, 2021
packages/components/src/toggle-group-control/toggle-group-control-option/component.tsx
Show resolved
Hide resolved
ciampo
reviewed
Nov 24, 2021
Contributor
ciampo
left a comment
There was a problem hiding this comment.
Thank you for the great work, Ramon! For some reason I must have been faster with my mouse than the 700ms tooltip delay 😅 I can confirm that it works as expected!
I've left a few more comments, but they're all minor, but we're very close!
9014929 to
0d716c4
Compare
7 tasks
diegohaz
approved these changes
Nov 25, 2021
Member
diegohaz
left a comment
There was a problem hiding this comment.
Great work @ramonjd! Thank you!
I wonder if we should just show the tooltips without a delay since they're semantically labels, and not just tips.
Anyway, tooltips are hard! Here's an excellent video that explains several problems with tooltips: https://youtu.be/lb0_v7D4kbs
We may also consider a design where both the number and the text are visible without the need of tooltips.
But this PR is already an improvement over the current version, so let's merge it! 🚀
To avoid the TS linter freaking out, we add a default of `null` for the shortcut. The 'position' prop, though helpful, is required for the same reason.
…olOption` to determine whether to show tooltip Update stories.
…he tooltip. Adding tests. Updated README.md for ToggleGroupControl Updated CHANGELOG.md Updated label condition Updated story Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> Removed unneeded export and __ import
Destructuring Tooltip props to hide them from the TS linter Updating tests to account for the Tooltip timers.
…loy useDebounce for now.
0d716c4 to
7c80be0
Compare
Member
Author
|
Thanks for the ✅ @ciampo and for the info and the helpful link @diegohaz
It looks like there's only a delay when we mouse over a tooltip. See: , wheretrue tells the event handler to debounce the tooltip.
Seems like a pretty easy change to make assuming there's consensus on removing it. Maybe the delay was there to reduce any perceived "clutter" for power speed mouse users like @ciampo and me Once the tests pass I'll 🚢 |
ramonjd
commented
Nov 25, 2021
noisysocks
pushed a commit
that referenced
this pull request
Nov 29, 2021
* Adding a ToolTip to the font-size-picker using the current label. To avoid the TS linter freaking out, we add a default of `null` for the shortcut. The 'position' prop, though helpful, is required for the same reason. * Allow a new prop `tooltipText` to be passed down to `ToggleGroupControlOption` to determine whether to show tooltip Update stories. * Changed the prop from text to a boolean in order to flag displaying the tooltip. Adding tests. Updated README.md for ToggleGroupControl Updated CHANGELOG.md Updated label condition Updated story Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> Removed unneeded export and __ import * Updated CHANGELOG.md Destructuring Tooltip props to hide them from the TS linter Updating tests to account for the Tooltip timers. * testing tooltip active state using the focus event, which doesn't employ useDebounce for now.
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
7 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.
Related:
Description
Hey ho.
This PR takes a stab at adding tooltips to the font size segmented control.
The gist is that we're giving the user additional information about their font-size selection only otherwise available via
aria-label(and not immediately visible in the browser).Here's a sneak preview:
Why? At present the font size segmented control displays numerical values that are not the same as their descriptions.
So
42is'Huge'.To achieve what we need, we've extended
ToggleGroupControlOptionto add a Tooltip wrapper around the radio component in the presence of atooltipTextprop.There might be other use cases for this, for example, adding extra descriptive power 🚀 or visible hints to options. The limit is your imagination!
How has this been tested?
npm run storybook:devTake a look at
FontSizePickerandToggleGroupControl(With Tooltip variation) : check that the tooltip appears.Check that there are no regressions for existing usage of
ToggleGroupControlOption. For example, inset a Navigation Block into the editor. The Overlay menu in the right-hand sidebar should look and work as expected. There should be no tooltips here.Another one is the Post Featured Image Block. Adjust the height dimensions and check the options group:
Run the unit test:
npm run test-unit packages/components/src/toggle-group-control/test/index.jsTypes of changes
Adding a feature to an existing component.
Checklist:
*.native.jsfiles for terms that need renaming or removal).