feat(slider): expose formatLabel to value tooltip #16616

dkaushik95 · 2024-05-29T17:14:51Z

This PR exposes formatLabel to the tooltip for devs to use

expose formatLabel to thumbWrapper for twoHandles as well as single handle.
add examples on storybooks on how it could be used

Doesn't close a PR but gets us closer to the comment here: #7581 (comment)

Changelog

New

formatLabel now also works with the Tooltips for the thumbs which gives you more options on how to customize how the value looks.
Two new storybooks added to describe the change

Changed

Changed the definition of formatLabel to represent the above addition

Testing / Reviewing

Ran unit test to make sure nothing is breaking
You can test it by changing the storybooks:
- Slider With Hidden Inputs And Custom Format
- Slider With Hidden Inputs And Format Label

Screenshots

github-actions · 2024-05-29T17:15:03Z

All contributors have signed the DCO.
_{Posted by the DCO Assistant Lite bot.}

netlify · 2024-05-29T17:20:27Z

✅ Deploy Preview for v11-carbon-react ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`668a37a`
🔍 Latest deploy log	https://app.netlify.com/sites/v11-carbon-react/deploys/6657628d440af80007f6eb8c
😎 Deploy Preview	https://deploy-preview-16616--v11-carbon-react.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

dkaushik95 · 2024-05-29T17:20:32Z

I have read the DCO document and I hereby sign the DCO.

netlify · 2024-05-29T17:20:42Z

✅ Deploy Preview for carbon-elements ready!

Name	Link
🔨 Latest commit	`668a37a`
🔍 Latest deploy log	https://app.netlify.com/sites/carbon-elements/deploys/6657628d8ebf4700088790d4
😎 Deploy Preview	https://deploy-preview-16616--carbon-elements.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify · 2024-05-29T17:26:00Z

✅ Deploy Preview for v11-carbon-react ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`1a98031`
🔍 Latest deploy log	https://app.netlify.com/sites/v11-carbon-react/deploys/668419639cfc3f0008100218
😎 Deploy Preview	https://deploy-preview-16616--v11-carbon-react.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify · 2024-05-29T17:26:35Z

✅ Deploy Preview for carbon-elements ready!

Name	Link
🔨 Latest commit	`1a98031`
🔍 Latest deploy log	https://app.netlify.com/sites/carbon-elements/deploys/66841963177e5500081aec5b
😎 Deploy Preview	https://deploy-preview-16616--carbon-elements.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

guidari · 2024-06-11T17:44:15Z

Hey @dkaushik95 sorry for the late reply.

This is something would be nice for us to talk with the design team, so we can delivery a consistent solution like it was mentioned here #7581 (comment)

I'll make sure to bring that discussion in further meetings with the team.

dkaushik95 · 2024-06-14T22:56:49Z

Thanks @guidari , please let me know if there's any other way we can achieve this without this change because we will be needing this functionality soon in our project.

bradelectro · 2024-06-17T17:01:26Z

@guidari This change will enable us to do the migration we need in our product, Instana. Can we please merge?

tay1orjones · 2024-06-17T20:12:14Z

This approach seems good to me. The design considerations on that issue are mostly related to the addition of increment "tics", which will still need a scalable solution for programatic/dynamic labels. I think this provides that pretty well. Seems like it would meet the needs of the increments feature if we do eventually take that on.

A few suggestions to get this closer to merge:

Add unit tests covering the new behavior
Condense both stories into one single story - I wonder if a name like "Slider With Custom Value Label" would make more sense to someone with no context on what the feature is or the props used?
Add that story to AVT and VRT - each of these should first move focus to one of the slider handles (so a tooltip is visible) before running toBeAccessible or snapshotStory
The aria-valuetext attribute needs to be placed on the input when using this label and the value should be updated just the same as the tooltip. Right now when using this with voiceover, only the numeric value is read while changing the value. You can get the "medium" to be read by pressing up or exiting the slider, but I think with aria-valuetext "medium" can be read instead or at least alongside/after the numeric value.

CleanShot.2024-06-17.at.14.56.42.mp4

bradelectro · 2024-06-18T19:37:47Z

@tay1orjones Thank you for being open to our suggestions. What is AVT / VRT please?

guidari · 2024-06-19T17:18:55Z

@tay1orjones Thank you for being open to our suggestions. What is AVT / VRT please?

Hey @bradelectro

AVT testing is located in this pathe2e/components/Slider/Slider-test.avt.e2e.js. This test helps us finding any a11y issues and also validate against keyboard navigation.
VRT testing is located in this path e2e/components/Slider/Slider-test.e2e.js. This test can guard us against any visual changes in the story.

I'm happy to help with this PR to get all checks that Taylor had mentioned.

tay1orjones · 2024-06-19T20:10:22Z

I had a conversation with @laurenmrice just now about the design-related concerns of this and we both agreed non-numeric labels are widely used enough (and supported in the a11y spec via aria-valuetext) that we should support it.

Also, If/when the increment/tics are implemented the intent is to allow the input to still be hidden, which means the dynamic tooltip value (the functionality in this PR) will be needed.

Once this PR is merged we'll revisit documentation on the website and determine how/if we should update it.

dkaushik95 · 2024-06-19T22:11:58Z

Thanks @tay1orjones and @guidari. I’ll complete the tests and fix the a11y issue and let you know once I’m done.

dkaushik95 · 2024-06-22T00:32:20Z

Here's what I've done:

Added unit test to cover the new behavior for both variants
Added VRT test using snapshotStory
Added AVT test by checking the label after pressing tab and using toHaveNoACViolations
Added the aria-valuetext and tested this by using VoiceOver on Mac. I was able to get the formatted values on change.

@guidari , @tay1orjones , can you please help me with verifying that the change works well? Thank you! :D

guidari

Hey!! The changes are looking good! Just a couple notes

e2e/components/Slider/Slider-test.avt.e2e.js

packages/react/src/components/Slider/Slider.stories.js

e2e/components/Slider/Slider-test.avt.e2e.js

guidari · 2024-06-28T12:44:15Z

e2e/components/Slider/Slider-test.avt.e2e.js

+    await expect(page).toHaveNoACViolations('Slider-with-custom-value-label');
+  });
+
+  test.slow('@avt-keyboard-nav slider with custom format', async ({ page }) => {


It is missing the visitStory here

Suggested change

test.slow('@avt-keyboard-nav slider with custom format', async ({ page }) => {

test.slow('@avt-keyboard-nav slider with custom format', async ({ page }) => {

await visitStory(page, {

component: 'Slider',

id: 'components-slider--slider-with-custom-value-label',

globals: {

theme: 'white',

},

});

Should be fixed now

guidari

LGTM! @dkaushik95
Thanks for the contribution!!

- expose formatLabel to thumbWrapper for twoHandles as well as single handle. - add examples on storybooks on how it could be used

- Avt, using tab and getting the value to show as medium - Snapshot test - Unit test to check the behavior for both single and two thumb variants

dkaushik95 requested a review from a team as a code owner May 29, 2024 17:14

dkaushik95 requested review from alisonjoseph and guidari May 29, 2024 17:14

alisonjoseph requested review from a team and kingtraceyj and removed request for a team May 29, 2024 17:45

dkaushik95 force-pushed the implement-formatLabel-on-tooltip branch 2 times, most recently from 8145068 to d8d2b82 Compare May 29, 2024 17:47

dkaushik95 force-pushed the implement-formatLabel-on-tooltip branch from d8d2b82 to f1ab421 Compare June 14, 2024 22:55

dkaushik95 force-pushed the implement-formatLabel-on-tooltip branch from d019f51 to 4955347 Compare June 22, 2024 00:28

dkaushik95 force-pushed the implement-formatLabel-on-tooltip branch from 4955347 to a4f9f55 Compare June 25, 2024 15:07

guidari requested changes Jun 25, 2024

View reviewed changes

e2e/components/Slider/Slider-test.avt.e2e.js Outdated Show resolved Hide resolved

packages/react/src/components/Slider/Slider.stories.js Outdated Show resolved Hide resolved

packages/react/src/components/Slider/Slider.stories.js Outdated Show resolved Hide resolved

dkaushik95 force-pushed the implement-formatLabel-on-tooltip branch from ec51780 to acf2770 Compare June 27, 2024 16:45

guidari requested changes Jun 27, 2024

View reviewed changes

e2e/components/Slider/Slider-test.avt.e2e.js Outdated Show resolved Hide resolved

dkaushik95 requested a review from guidari June 27, 2024 17:34

guidari requested changes Jun 28, 2024

View reviewed changes

dkaushik95 requested a review from guidari June 28, 2024 20:27

dkaushik95 force-pushed the implement-formatLabel-on-tooltip branch from 92914b1 to a155394 Compare June 28, 2024 20:27

guidari approved these changes Jul 1, 2024

View reviewed changes

guidari added the status: one more review 👀 label Jul 1, 2024

dkaushik95 added 5 commits July 2, 2024 08:14

feat(slider): expose formatLabel to value tooltip

fe3904a

- expose formatLabel to thumbWrapper for twoHandles as well as single handle. - add examples on storybooks on how it could be used

test(slider): add unit, avt, and e2e tests on slider for new behavior

5f4647c

- Avt, using tab and getting the value to show as medium - Snapshot test - Unit test to check the behavior for both single and two thumb variants

test(slider): add suggested changes to the slider test and stories

d7a41ab

test(slider): move two types of test separate to each other

e395e28

test(slider): fix avt e2e test

1a98031

dkaushik95 force-pushed the implement-formatLabel-on-tooltip branch from a155394 to 1a98031 Compare July 2, 2024 15:14

alisonjoseph added the status: visual review 🔍 label Jul 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(slider): expose formatLabel to value tooltip #16616

feat(slider): expose formatLabel to value tooltip #16616

dkaushik95 commented May 29, 2024

github-actions bot commented May 29, 2024 •

edited

Loading

netlify bot commented May 29, 2024

dkaushik95 commented May 29, 2024

netlify bot commented May 29, 2024

netlify bot commented May 29, 2024 •

edited

Loading

netlify bot commented May 29, 2024 •

edited

Loading

guidari commented Jun 11, 2024

dkaushik95 commented Jun 14, 2024

bradelectro commented Jun 17, 2024

tay1orjones commented Jun 17, 2024

bradelectro commented Jun 18, 2024

guidari commented Jun 19, 2024

tay1orjones commented Jun 19, 2024 •

edited

Loading

dkaushik95 commented Jun 19, 2024

dkaushik95 commented Jun 22, 2024

guidari left a comment

guidari Jun 28, 2024

dkaushik95 Jun 28, 2024

guidari left a comment

-  test.slow('@avt-keyboard-nav slider with custom format', async ({ page }) => {
+  test.slow('@avt-keyboard-nav slider with custom format', async ({ page }) => {
+  await visitStory(page, {
+      component: 'Slider',
+      id: 'components-slider--slider-with-custom-value-label',
+      globals: {
+        theme: 'white',
+      },
+    });

feat(slider): expose formatLabel to value tooltip #16616

Are you sure you want to change the base?

feat(slider): expose formatLabel to value tooltip #16616

Conversation

dkaushik95 commented May 29, 2024

Changelog

Testing / Reviewing

Screenshots

github-actions bot commented May 29, 2024 • edited Loading

netlify bot commented May 29, 2024

✅ Deploy Preview for v11-carbon-react ready!

dkaushik95 commented May 29, 2024

netlify bot commented May 29, 2024

✅ Deploy Preview for carbon-elements ready!

netlify bot commented May 29, 2024 • edited Loading

✅ Deploy Preview for v11-carbon-react ready!

netlify bot commented May 29, 2024 • edited Loading

✅ Deploy Preview for carbon-elements ready!

guidari commented Jun 11, 2024

dkaushik95 commented Jun 14, 2024

bradelectro commented Jun 17, 2024

tay1orjones commented Jun 17, 2024

bradelectro commented Jun 18, 2024

guidari commented Jun 19, 2024

tay1orjones commented Jun 19, 2024 • edited Loading

dkaushik95 commented Jun 19, 2024

dkaushik95 commented Jun 22, 2024

guidari left a comment

Choose a reason for hiding this comment

guidari Jun 28, 2024

Choose a reason for hiding this comment

dkaushik95 Jun 28, 2024

Choose a reason for hiding this comment

guidari left a comment

Choose a reason for hiding this comment

github-actions bot commented May 29, 2024 •

edited

Loading

netlify bot commented May 29, 2024 •

edited

Loading

netlify bot commented May 29, 2024 •

edited

Loading

tay1orjones commented Jun 19, 2024 •

edited

Loading