Fix/a11y file input 1.1.1 v3 #7679
Conversation
src/js/languages/default.json
Outdated
| "alert": { | ||
| "maxSize": "Error - remove file: {fileName} to continue. This file is too large. Attach files which are no larger than {maxSize}.", | ||
| "maxFile": "Error - remove file: {fileName} to continue. Attach a maximum of {max} files only." | ||
| } |
There was a problem hiding this comment.
same. I still think we need to avoid saying "Error - remove file"
| "alert": { | |
| "maxSize": "Error - remove file: {fileName} to continue. This file is too large. Attach files which are no larger than {maxSize}.", | |
| "maxFile": "Error - remove file: {fileName} to continue. Attach a maximum of {max} files only." | |
| } | |
| "alert": { | |
| "maxSize": "File {fileName} is too large. Attach files which are no larger than {maxSize}.", | |
| "maxFile": "Attach a maximum of {max} files only. File {fileName} is beyond this limit" | |
| } |
There was a problem hiding this comment.
I kept the "Error - remove file:..." because of last review I got from Bill below.
"Hello Sulaymon,
I like option 1.
You can learn the reason in a recording I did for both versions at the link below.
One suggestion: Although the button in focus says Remove: when telling the user about the error you may want to tell them at the end of that sentence what action must be done to proceed. Such as the file must be removed or something close to that. Then the user knows there is an error and what is needed to continue."
I have shared this version with him as well and I am expecting his evaluation when he is bank next week.
There was a problem hiding this comment.
I kept the "Error - remove file:..." because of last review I got from Bill below.
Thanks for review with Bill. That's awesome. I think we can achieve the same thing but avoiding saying it as "Error ...." It's actually one of the standards across a number of our UX folks to avoid that kind of wording. I appreciate the comment from Bill and I'm sure we can still word this well but avoid conflicting with other messaging guidance that's more global to our product base.
There was a problem hiding this comment.
Thank you for the review. that makes sense. the alert messages are now updated.
There was a problem hiding this comment.
@MikeKingdom I do get your comment about using the word error Bill just mentioned that its nice to know that something is wrong right away instead of listening for the screen reader to finish the filename (especially if its long) to then say the file was to large. So I wonder if we can do something like
This file {name} is too large to upload. Remove it to continue.
File is too large. 'filename.pdf' must be removed to continue.
There was a problem hiding this comment.
@britt6612 thank you for the comment.
I would add something like Alert or Attention at the beginning of the text and possibly mention the max size as well.
So something like
maxSize: Attention, this file {name} is too large. Attach files which are no larger than {maxSize} to continue.
maxFile: Attention, attach a maximum of {max} files only. File {fileName} is beyond this limit.
For maxFile, I believe with the tone above, we are allowing the user to decide what they would like to do. Like Mike pointed out earlier in one of the previous comments for when users attached too many files.
There was a problem hiding this comment.
What caused these diffs? Did we change a theme color somewhere?
There was a problem hiding this comment.
I did not make any changes to theme color.
There was a problem hiding this comment.
Please see if you can determine why.
There was a problem hiding this comment.
OK. I am looking into it more closely.
What does this PR do?
Where should the reviewer start?
What testing has been done on this PR?
How should this be manually tested?
Do Jest tests follow these best practices?
screenis used for querying.asFragment()is used for snapshot testing.Any background context you want to provide?
What are the relevant issues?
Screenshots (if appropriate)
Do the grommet docs need to be updated?
Should this PR be mentioned in the release notes?
Is this change backwards compatible or is it a breaking change?
Yes