department-of-veterans-affairs / vets-design-system-documentation

Repository for design.va.gov website
https://design.va.gov
36 stars 55 forks source link

Files pattern should include confirmation step when deleting #1770

Closed briandeconinck closed 5 months ago

briandeconinck commented 1 year ago

Duplicate check

This update is for:

Pattern

What is the name?

Files

What is the nature of this update?

What problem does this solve?

The Files pattern includes examples for:

I would like to add a "Delete state" that shows the preferred pattern for deleting an uploaded file. In this pattern, users would be prompted to confirm the delete action (with an option to cancel) before the file is actually deleted. I think the most straightforward implementation of this would be with a modal, but I'm open to other options as well!

Additionally, I would like to add the following to the accessibility guidance on the pattern (please edit as appropriate!):

  • Ask for confirmation when deleting files. Destructive actions like deleting files should require two steps by users --- the initial button click, and then a confirmation. This helps prevent users from accidentally deleting a file with an unintentional click, and provides an extra prompt for screen reader users and screen magnification users who might not see the visual change when a file is removed.

Additional Context

This came up in a staging review for Supplemental Claims Form 20-0995, and @Mottie is going to be working on it and may be able to provide some good screenshots. 😄

See also WCAG SC 3.3.4 and WCAG Technique G168. A form that doesn't confirm file deletion probably isn't technically a 3.3.4 violation since users will have an opportunity to review before final submission... but it definitely falls under the spirit of 3.3.4 and would be good to address.

Mottie commented 11 months ago

I've updated the FileField component, but I didn't switch the input to a va-file-input because React Testing Library (RTL) doesn't work well with shadow DOM - The PR was already huge enough updating the React component to a functional component, and updating the unit tests to RTL. Maybe I'll tackle the change to va-file-input in the future.

Also note: File upload code outside of platform (COE & Claim Status Tool) have not yet been updated.

Mottie commented 11 months ago

@briandeconinck Josh is on PTO until 7/24, if you would please test in his place? Our forms that have uploads are:

You can log in to user 228 (Colder). Note that non of our mock users currently include any (loaded) contestable issues in staging.

briandeconinck commented 11 months ago

@Mottie This looks good!

I am noticing that when more than one file has been uploaded, the "Delete file" buttons don't have unique accessible names --- they're both "Delete file." Would love for them to be "Delete [filename]" or have an aria-label to make it easier for screen reader users to differentiate the files.

But that's outside the scope of this pattern. The actual behavior you were working on all looks good! 😄

Mottie commented 11 months ago

Would love for them to be "Delete [filename]" or have an aria-label to make it easier for screen reader users to differentiate the files.

Hmmm, the outer va-button includes a aria-describedby pointing to the filename

<va-button
  secondary
  class="delete-upload vads-u-width--auto hydrated"
  aria-describedby="root_additionalDocuments_file_name_0"
  text="Delete file" />

I guess it doesn't work on the outer wrapper. I'll make a quick update to add a label (adds an aria-label to the internal button) instead. Thanks for looking!

Mottie commented 11 months ago

@briandeconinck Please test the button label updates 😸

Mottie commented 10 months ago

Nudge @briandeconinck - this work should be done. Please test at your convenience

humancompanion-usds commented 5 months ago

@allison0034 - May I trouble you to gather a screenshot or 2 for this issue? Thanks! I'm making the guidance update now.

Mottie commented 5 months ago

Will the screenshots from this PR work?

allison0034 commented 5 months ago

@Mottie I think that would work but the per the content considerations for that component we should use file instead of document. And after looking at the other screen shots in our DS most of them have documents. @humancompanion-usds I don't want to scope creep, but can we replace those as well?

humancompanion-usds commented 5 months ago

@allison0034 - If you can source an example then I'm happy to add it. But otherwise I'm going to use the one from Supplemental Claims.

allison0034 commented 5 months ago

@humancompanion-usds would these work? They are from a Sketch file. This team used a white card instead of gray also.

Screenshot 2024-01-09 at 10 39 17 AM Screenshot 2024-01-09 at 10 39 37 AM
humancompanion-usds commented 5 months ago

@allison0034 - If this has shipped and we're sure of that then yes I don't mind using the Sketch file. However, we want to also show the modal that opens when you click "Delete file". Is that in there as well?

Mottie commented 5 months ago

Oh, I think the content was changed from "document" to "file" - the screenshots are likely out of date

allison0034 commented 5 months ago

@humancompanion-usds I worked with Robin to get some screens that have the correct content and the modal in production. Let me know if you need more. 1-file-upload_ReqBoardApp 1-file-upload_SuppClaim

2-file-upload

2-file-upload-SuppClaim

3-2-file-upload-SuppClaim

3-file-upload-SuppClaim