2777 Add restProps to IconButton … #2791

emilyuhde · 2022-12-12T01:32:44Z

… and use it to pass aria-labels down to the button element where it's used in other UI components

@pngwn

Description

relevant motivation - Button must have discernible text, per WCAG success criterion 4.1.2 https://www.w3.org/WAI/WCAG21/Understanding/name-role-value.html
a summary of the change - restProps was added to the element of the IconButton atom component, this allows for aria-labels to be passed down to the element.

Closes: #2777

Checklist:

I have performed a self-review of my own code
I have added a short summary of my change to the CHANGELOG.md
My code follows the style guidelines of this project
I have commented my code in hard-to-understand areas
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes

A note about the CHANGELOG

Hello 👋 and thank you for contributing to Gradio!

All pull requests must update the change log located in CHANGELOG.md, unless the pull request is labeled with the "no-changelog-update" label.

Please add a brief summary of the change to the Upcoming Release > Full Changelog section of the CHANGELOG.md file and include
a link to the PR (formatted in markdown) and a link to your github profile (if you like). For example, "* Added a cool new feature by [@myusername](link-to-your-github-profile) in [PR 11111](https://github.com/gradio-app/gradio/pull/11111)".

If you would like to elaborate on your change further, feel free to include a longer explanation in the other sections.
If you would like an image/gif/video showcasing your feature, it may be best to edit the CHANGELOG file using the
GitHub web UI since that lets you upload files directly via drag-and-drop.

…to the button element where it's used in other UI components

emilyuhde · 2022-12-12T01:35:25Z

Just a note on the changelog, my IDE is set to automatically remove trailing whitespace from files on save, hence all the changes. I can try to revert that if you want to keep them.

Also, please let me know if there's something I need to do to get these hooked up for translations!

pngwn

Hi @emilyuhde. Thanks for this!

Just left some comments around the use of $$restProps. When that is adjusted we should be good to go.

This PR will also need to add an item to the CHANGELOG.md.

Under the Upcoming Release heading you need to add something like the following

## Bug Fixes

* Add aria-labels to button icons, improving accessibility by [@emilyuhde](https://github.com/emilyuhde) in [in PR2791](https://github.com/gradio-app/gradio/pull/2791)

Don't worry too much about the conflicts. When this PR is ready, I can resolve those.

pngwn · 2022-12-13T17:07:28Z

ui/packages/atoms/src/IconButton.svelte

@@ -6,6 +6,7 @@
 <button
 	on:click
 	class="text-gray-500  bg-white/90 h-5 w-5 flex items-center justify-center rounded shadow-sm hover:shadow-xl hover:ring-1 ring-inset ring-gray-200 z-10 dark:bg-gray-900 dark:ring-gray-600"
+	{...$$restProps}


I'm not a huge fan of $$restProps because it is hard to work out what is being passed down and results in code that isn't as well optimised (by the Svelte compiler). It is useful when we don't know the props at build time, but that isn't the case here.

Since we know these values ahead of time, we can just define a label prop in this component (export let label) and set that as the aria-label explicitly.

pngwn · 2022-12-13T17:08:12Z

ui/packages/image/src/ModifySketch.svelte

@@ -7,9 +7,10 @@
 </script>

 <div class="z-50 top-2 right-2 justify-end flex gap-1 absolute">
-	<IconButton Icon={Undo} on:click={() => dispatch("undo")} />
+	<IconButton Icon={Undo} aria-label="Undo" on:click={() => dispatch("undo")} />


Follwing on from the above, when that is done we will need to change this to label=....

pngwn · 2022-12-13T17:08:24Z

ui/packages/image/src/SketchSettings.svelte

-		<IconButton Icon={Brush} on:click={() => (show_size = !show_size)} />
+		<IconButton
+			Icon={Brush}
+			aria-label="Use brush"


ui/packages/image/src/SketchSettings.svelte

pngwn · 2022-12-13T17:08:37Z

ui/packages/image/src/SketchSettings.svelte

-			<IconButton Icon={Color} on:click={() => (show_col = !show_col)} />
+			<IconButton
+				Icon={Color}
+				aria-label="Select brush color"


ui/packages/image/src/SketchSettings.svelte

pngwn · 2022-12-13T17:08:51Z

ui/packages/upload/src/ModifyUpload.svelte

-		<IconButton Icon={Edit} on:click={() => dispatch("edit")} />
+		<IconButton
+			Icon={Edit}
+			aria-label="Edit"


pngwn · 2022-12-13T17:08:56Z

ui/packages/upload/src/ModifyUpload.svelte

 	{/if}

 	<IconButton
 		Icon={Clear}
+		aria-label="Clear"


emilyuhde · 2022-12-13T17:48:52Z

Hi @emilyuhde. Thanks for this!

Just left some comments around the use of $$restProps. When that is adjusted we should be good to go.

This PR will also need to add an item to the CHANGELOG.md.

Under the Upcoming Release heading you need to add something like the following
## Bug Fixes

* Add aria-labels to button icons, improving accessibility by [@emilyuhde](https://github.com/emilyuhde) in [in PR2791](https://github.com/gradio-app/gradio/pull/2791)
Don't worry too much about the conflicts. When this PR is ready, I can resolve those.

No problem @pngwn!

I can change the $$restProps to being more explicit, no problem. I wasn't sure if you'd have a preference on this so I went with what I'd normally do for a React component in a library to give as much flexibility as possible.
I do have a Changelog line in there; it's probably hard to notice because of the giant diff.

I just sync-ed my fork, I see the conflict and can resolve it

Also, do I need to do anything to get translations hooked up so these strings aren't hard coded?

… using restProps

…yuhde/gradio into issue-2777-iconbuttonlabels

pngwn · 2022-12-14T11:54:04Z

I don't understand why the changelog check is failing. @freddyaboulton Do you have any insight here?

abidlabs · 2022-12-14T13:07:00Z

It's because the PR number in the changelog is #2777, which corresponds to the issue, whereas it should correspond to the PR itself. If we change that to #2791, it should pass.

Thanks for the PR @emilyuhde!

emilyuhde · 2022-12-14T13:24:53Z

It's because the PR number in the changelog is #2777, which corresponds to the issue, whereas it should correspond to the PR itself. If we change that to #2791, it should pass.

Thanks for the PR @emilyuhde!

You're welcome. Got it, I'll fix that right now.

pngwn

Looks great, thanks for this @emilyuhde !

pngwn · 2022-12-14T21:33:32Z

I should not have done that.

CHANGELOG.md

pngwn · 2022-12-14T21:35:09Z

i think it is okay now.

emilyuhde · 2022-12-14T21:57:20Z

I'm sure this got lost in the mess, but do I need to do anything so these strings can be translated instead of hard coded in as English? @pngwn

pngwn · 2022-12-14T22:06:10Z

Sorry I did mean to follow that up, my bad.

pngwn · 2022-12-14T22:11:12Z

I just double checked, we don't really have a good mechanism for passing translations that far down (without it getting messy) nor do we have translations for those labels either. i18n is one of thing we are planning to overhaul next year, so i think it is okay to merge this in and deal with i18n a little later so we have some time to figure out a better design (than what we have now).

emilyuhde · 2022-12-14T22:13:03Z

Ok that sounds good then, just wanted to check before this was merged in!

pngwn · 2022-12-14T22:13:26Z

Thanks again @emilyuhde!

2777 Add restProps to IconButton and use it to pass aria-labels down …

0b387fe

…to the button element where it's used in other UI components

pngwn requested changes Dec 13, 2022

View reviewed changes

emilyuhde added 2 commits December 13, 2022 13:07

2777 Update per PR feedback, explicitly pass in label prop instead of…

a15d7d8

… using restProps

Merge branch 'main' into issue-2777-iconbuttonlabels

bf2038a

emilyuhde requested a review from pngwn December 13, 2022 18:15

emilyuhde added 3 commits December 13, 2022 14:54

Merge branch 'main' into issue-2777-iconbuttonlabels

0deb574

2777 Fix mysterious linter error

f3d2ff4

Merge branch 'issue-2777-iconbuttonlabels' of https://github.com/emil…

314e5dd

…yuhde/gradio into issue-2777-iconbuttonlabels

emilyuhde added 2 commits December 14, 2022 08:26

Update changelog to fix PR number 2791

dad2bfa

Fix merge conflict

3f26d36

pngwn approved these changes Dec 14, 2022

View reviewed changes

Merge branch 'main' into issue-2777-iconbuttonlabels

88bfebd

pngwn reviewed Dec 14, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Update CHANGELOG.md

351ebd4

pngwn merged commit ade918d into gradio-app:main Dec 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2777 Add restProps to IconButton … #2791

2777 Add restProps to IconButton … #2791

emilyuhde commented Dec 12, 2022 •

edited

Loading

emilyuhde commented Dec 12, 2022 •

edited

Loading

pngwn left a comment •

edited

Loading

pngwn Dec 13, 2022

pngwn Dec 13, 2022

pngwn Dec 13, 2022

pngwn Dec 13, 2022

pngwn Dec 13, 2022

pngwn Dec 13, 2022

emilyuhde commented Dec 13, 2022 •

edited

Loading

pngwn commented Dec 14, 2022

abidlabs commented Dec 14, 2022

emilyuhde commented Dec 14, 2022

pngwn left a comment

pngwn commented Dec 14, 2022

pngwn commented Dec 14, 2022

emilyuhde commented Dec 14, 2022

pngwn commented Dec 14, 2022

pngwn commented Dec 14, 2022

emilyuhde commented Dec 14, 2022

pngwn commented Dec 14, 2022

2777 Add restProps to IconButton … #2791

2777 Add restProps to IconButton … #2791

Conversation

emilyuhde commented Dec 12, 2022 • edited Loading

Description

Checklist:

A note about the CHANGELOG

emilyuhde commented Dec 12, 2022 • edited Loading

pngwn left a comment • edited Loading

Choose a reason for hiding this comment

pngwn Dec 13, 2022

Choose a reason for hiding this comment

pngwn Dec 13, 2022

Choose a reason for hiding this comment

pngwn Dec 13, 2022

Choose a reason for hiding this comment

pngwn Dec 13, 2022

Choose a reason for hiding this comment

pngwn Dec 13, 2022

Choose a reason for hiding this comment

pngwn Dec 13, 2022

Choose a reason for hiding this comment

emilyuhde commented Dec 13, 2022 • edited Loading

pngwn commented Dec 14, 2022

abidlabs commented Dec 14, 2022

emilyuhde commented Dec 14, 2022

pngwn left a comment

Choose a reason for hiding this comment

pngwn commented Dec 14, 2022

pngwn commented Dec 14, 2022

emilyuhde commented Dec 14, 2022

pngwn commented Dec 14, 2022

pngwn commented Dec 14, 2022

emilyuhde commented Dec 14, 2022

pngwn commented Dec 14, 2022

emilyuhde commented Dec 12, 2022 •

edited

Loading

emilyuhde commented Dec 12, 2022 •

edited

Loading

pngwn left a comment •

edited

Loading

emilyuhde commented Dec 13, 2022 •

edited

Loading