department-of-veterans-affairs / va.gov-team

Public resources for building on and in support of VA.gov. Visit complete Knowledge Hub:
https://depo-platform-documentation.scrollhelp.site/index.html
281 stars 198 forks source link

[508] Use button for upload #36877

Closed Mottie closed 1 day ago

Mottie commented 2 years ago

Description

The upload button currently being used is a <label> wrapping a <span> set to a role of button. The input file type is hidden. Given that this is to restyle the default upload button, I don't think we need to include a <label> nor use a <span> as a button.

Question: Does a hidden file input need an associated label?

Dev tools showing hidden file input with a span with role type button inside a label next to it

This pattern can be found in:

Also, we shouldn't be using inline styling (style={{ display: 'none' }}) on the file input

Tasks

Definition of done

humancompanion-usds commented 2 years ago

@k80bowman and @bkjohnson - Looks to me like this may be fixed by using File Input (https://design.va.gov/storybook/?path=/docs/components-fileinput--default). Some questions:

Mottie commented 2 years ago

Actually, within the form system, the FileField component is used, and it's using a <span> as a button, as shown in the screenshot

<label
  id={`${idSchema.$id}_add_label`}
  htmlFor={idSchema.$id}
  className="vads-u-display--inline-block"
>
  <span
    role="button"
    className="usa-button usa-button-secondary vads-u-padding-x--2 vads-u-padding-y--1"
    onKeyPress={e => {
      e.preventDefault();
      if (['Enter', ' ', 'Spacebar'].indexOf(e.key) !== -1) {
        this.fileInputRef.current.click();
      }
    }}
    tabIndex="0"
    aria-label={`${buttonText} ${titleString}`}
  >
    {buttonText}
  </span>
</label>
bkjohnson commented 2 years ago

Why is File Input not documented in design.va.gov? Is that intentional or oversight?

As far as I know that's not intentional - it must have been an oversight (but I did find documentation on a file upload pattern). We do plan on creating a web component for it, but it's a low priority at the moment.

As Robin points out, this is an example of the forms system using its own components rather than things provided by the design system. It's possible that we could swap out the forms system's FileField for our own FileInput, but there may be application unit or e2e tests which use selectors or other things specific to FileField.

For example, here's a cypress test which targets the .schemaform-file-uploading class which is used in the FileField definition. Removing that component and replacing it with our own could cause tests to start failing, or give false positives.

Another factor which complicates the idea of swapping out forms system internals for design system components is that many of those form system fields/widgets are tied to react-jsonschema-form. It's not just an <input type="file" /> with a label and some styling - it's also got many schema and uiSchema specific things attached to it.

humancompanion-usds commented 2 years ago

Thanks! I think I follow now. This could potentially be considered another forms related (adjacent?) web-component that the Forms System could consume from us, though they certainly don't need it in the immediate future. Still, we said we'd prioritize what we do for that team so that we're not blocking them. Let's discuss on Monday.

caw310 commented 1 day ago

@Mottie is this still an issue?

Mottie commented 1 day ago

Platform FileField is now using a label that wraps a va-button, so I think we can close this ticket. Eventually, it should be updated to use a va-file-input-multiple web component, but there are still some problems with that web component that need to be addressed