Native file upload component #1807
Open
+70
−59
Conversation
This file contains 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
amazingphilippe
commented
Apr 16, 2024
amazingphilippe
commented
Apr 16, 2024
amazingphilippe
changed the title
🧪 Review environmenthttps://himtr4theymwmv7ch7jkst6um40ridha.lambda-url.ca-central-1.on.aws/ |
…into a11y/file-buttons
This reverts commit 877b4b8.
5 tasks
andrewleith
requested changes
May 29, 2024
| @@ -23,7 +23,7 @@ | |||
| const input_img = document.querySelector("input.file-upload-field"); | |||
| const alt_en = document.getElementById("alt_text_en"); | |||
| const alt_fr = document.getElementById("alt_text_fr"); | |||
| const message = document.querySelector(".preview .message"); | |||
| const message = document.querySelector("#file-description"); | |||
There was a problem hiding this comment.
Suggested change
| const message = document.querySelector("#file-description"); | |
| const message = document.getElementById("file-description"); |
| {% endif %} | ||
|
|
||
| <div class="form-group"> | ||
| {{ form.file(**{ 'class': 'file-upload-field', 'accept': 'image/png', 'data_testid': 'brand_image', 'aria-labelledby': 'file-description', 'data-error-msg': _('You must select a file to continue') }) }} |
There was a problem hiding this comment.
We need to have the data-testid brand_image for the tests to continue to pass. Unfortunately the tests still arent merged so this would have been easy to miss.
There was a problem hiding this comment.
I can add it back. The testid needs to be on the input element?
There was a problem hiding this comment.
Added testid as file field optional attribute. defaults to test_field-name
whabanks
requested changes
May 29, 2024
There was a problem hiding this comment.
I noticed a couple of issues:
- On the branding upload page I am unable to "click" the
File Uploadbutton with the keyboard both with and without a screen reader. - When using a screen reader the
File Uploadbutton is not read out
REC-20240529093739.mp4
- On the
Prepare a spreadsheetpage the File upload button cannot be navigated to via the keyboard. - There's some extra hint text showing the mime types / file extensions accepted that I don't see captured in your original screen shots, was this an intentional change?
REC-20240529102806.mp4
|
Yikes! Thanks Will for spotting these issues.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary | Résumé
For cds-snc/notification-planning#1229
Fundamentally reconsiders how we do file uploaders.
More native, with a bit of css.
Can't be localized: The browser's default file uploader is sort of a web component, and you can't slot in your own text strings.
At the minimum, lets do some user testing with this PR.
Test instructions | Instructions pour tester la modification
There will be the following visual changes:
Logo before:
Logo after:
Check the following behaviours for Logo form:
Send before:
Send after:
Check the following on the send flow: