feat: Adding necessary labels to increase accessibility of screens #2755
Conversation
|
@stackingsaunter @rolznz Can you review this PR. |
| @@ -16,6 +16,10 @@ function BalanceBox({ className }: Props) { | |||
| "w-full flex flex-col items-center justify-center dark:text-white p-4", | |||
| className ?? "" | |||
| )} | |||
| tabIndex={0} | |||
| aria-label={`Balance: ${balancesDecorated.accountBalance}${ | |||
There was a problem hiding this comment.
why do we use aria-label here and not where the balance is rendered?
could a simple label like "balance" be added to the div that actually renders the value?
There was a problem hiding this comment.
We can add the aria-label to the BalanceBox component directly at the place where it is rendering I have added it in the div inside the component so that at every place the BalanceBox component is being used the screen reader can detect it but ig in this case BalanceBox is used only at one place so I will add it there itself.
I did not get your second point here, can you elaborate a bit?
There was a problem hiding this comment.
basically we just need to describe what the value means, right? We don't have to add a label which duplicates the actual content that we already render?
As far as I understand, this is for cases where there is just a number with no context for visually-impaired people.
There was a problem hiding this comment.
Yes, but in this case by using the aria-label attribute we are erasing the default label (which is the child elements' text) and creating a new label which replaces the default label rather than adding/extending to it. So that is hy there is a need of adding the data again to the aria-label.
There was a problem hiding this comment.
@Rithvik-padma I do not believe that is true. aria-label does not replace anything, it just enhances and provides more information.
A simple example would be:
<div aria-label="Wallet Balance">{walletBalance}</div>
A visually impaired user does not know the number inside the div is the balance. But the aria-label enhances that div and reads both the value, and the label.
There was a problem hiding this comment.
@rolznz
No, I felt it does the same initially but I have tested this format using a screen reader but this doesn't work, the screen reader only reads only the aria-label value but not the value inside it.
These are the resources I have referred to confirm - Stackoverflow, Medium post and also MDN docs.
Anyway there is an alternative which is using a label at the necessary places and making it visually hidden using the sr-only class provided by tailwindcss. I have tested this method using screen reader, it works fine and looks like a better solution for labels.
Should I use this method instead of using aria-label attribute?
There was a problem hiding this comment.
@rolznz No, I felt it does the same initially but I have tested this format using a screen reader but this doesn't work, the screen reader only reads only the aria-label value but not the value inside it. These are the resources I have referred to confirm - Stackoverflow, Medium post and also MDN docs.
Anyway there is an alternative which is using a label at the necessary places and making it visually hidden using the
sr-onlyclass provided by tailwindcss. I have tested this method using screen reader, it works fine and looks like a better solution for labels. Should I use this method instead of usingaria-labelattribute?
@rolznz any thoughts on this?
There was a problem hiding this comment.
@Rithvik-padma I like the suggestion in the stack overflow answer you linked:
<p><span class="visually-hidden">Participant</span> Martin Clark</p>
There is support in tailwind for this: https://tailwindcss.com/docs/screen-readers
That way we can remove some of the duplication. What do you think?
There was a problem hiding this comment.
Yes I will make changes for it
|
@rolznz I have made the changes as per your review. |
Before the changes can be tested we need to make sure we do this right. I think there needs some more changes first. |
Sure got it! |
|
@rolznz @stackingsaunter any update on this? |
|
@stackingsaunter any views on this PR? |
|
There seem to be quite some merge conflicts and also all of the accessibility strings seem to miss translations 🤔 Can you update this PR and add them (try to use existing translations wherever possible) |
|
@Rithvik-padma sorry to keep you waiting that long for the review! I can't test it right now, but will gladly do with fixes that @rolznz and @reneaaron mentioned |
Describe the changes you have made in this PR
Added labels at various places in the following screens -
Link this PR to an issue [optional]
Fixes #2573
Type of change
feat: New feature (non-breaking change which adds functionality)Screenshots of the changes [optional]
Add screenshots to make your changes easier to understand. You can also add a video here.
How has this been tested?
Tested the labels using a screen reader but few screens have not been tested as I am using a testnet (send and result screens).
Checklist