Skip to content
This repository has been archived by the owner on Aug 26, 2021. It is now read-only.

[DRAFT] File upload component #680

Closed
wants to merge 3 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Mar 10, 2019

File upload component draft. #613

Contains basic implementation on vanilla module for single file upload and multiple upload with display of selected files.

To do:

  1. Styles. Need some sketches for that component.
  2. Functionality. Public API, other features like auto send files on server, errors etc.
  3. Other stuff

@azerella azerella added enhancement New feature or request. in progress Currently working on. help wanted Extra attention is needed. new component An issue or pull request related to a new component. labels Mar 11, 2019
@azerella azerella added this to the Sprint 125 milestone Mar 11, 2019
@azerella
Copy link
Contributor

azerella commented Mar 11, 2019

@toxamiz Thanks for tackling this component! It will probably take us a week or so to go over but face value - looking good 👍

packages/file-upload/package.json Outdated Show resolved Hide resolved
packages/file-upload/package.json Outdated Show resolved Hide resolved
"@gov.au/core": "^3.0.0",
"@gov.au/buttons": "^3.0.6"
},
"devDependencies": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We pushed through a pretty large change only yesterday where we significantly bumped up the dependencies we use across all packages / scripts.

It's bad timing I'd say, can you please rebase/merge this branch with the latest develop

packages/file-upload/package.json Outdated Show resolved Hide resolved
@sukhrajghuman
Copy link
Contributor

sukhrajghuman commented Mar 13, 2019

Hey @toxamiz this is a great first PR!

A few things I picked up:

  1. Any reason why a label is being used instead of say a button for the file upload input selector? I think this does not follow a11y and semantic html. I wouldn't think to click text in a form to trigger the upload file popup. I'd try to do something like this:
<div class="au-file-upload">
  <label class="au-label">Attach drivers license</label>
  <input class="au-file-upload__input" type="file" />
  <button class="au-btn au-btn--secondary">Choose file</button>
  <span class="No file chosen" />
</div>

So it could look something like this in the context of the form:

It could even be worth using au-btn--tertiary for the choose file button. This is because a form page may contain a secondary button for clearing the form or going to a previous page.


  1. I'd hide all the custom styling when javascript is disabled, since it gets in the way of the browser default file input.

image

We add a .no-js class when javascript is not loaded on a page. You can use a selector that goes something like this:

.au-file-upload {

.no-js & {
//insert styles here
}

This is a good example as well:

// Hide the accordion title without javascript
.no-js & {
display: none;
}


  1. There are a couple of problems with the multiple selector:
    You are using a primary button for the remove file option:

image

This can be confusing, especially if this is contained within a form that has a primary button that submits it.

I would probably remove that functionality to remove altogether and just stick to the default functionality for this, which replaces the previous files selected with the new files selected.

Also we make sure all of our components are supported back to IE8. I'm not sure if multiple file feature is supported in < IE10? If not, do you know of any work arounds?


  1. I would probably even break this PR up. We could release an alpha version, that just contains a single file upload component. And possibly build on it with multiple file upload component, since it's easier to manage that way.

Let me know if this doesn't make sense.


<h1>Test: file-upload</h1>

<div class="au-file-upload js-upload-1">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use id instead of class for the js-upload-1 selector

</div>


<div class="au-file-upload js-upload-2">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<div class="au-file-upload js-upload-2">
<div class="au-file-upload" id="js-upload-2">

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. It uses querySelector, you can pass an id selector AU.fileUpload('#js-upload-2')

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea I saw that, but the main reason is because we won't use the .js-upload-2 selector on another element, so it's better to use #js-upload-2 if that makes sense.

@ghost ghost force-pushed the feature/file-upload branch from ca54159 to b8555c6 Compare March 13, 2019 18:21
@ghost
Copy link
Author

ghost commented Mar 13, 2019

Branch rebase on latest develop now. Added email :)

@adamzerella Do i need to resolve conflicts in auds.json? Or i can simply accept all from develop and then run npm i in package directory?

@ghost
Copy link
Author

ghost commented Mar 13, 2019

@sukhrajghuman

  1. Any reason why a label is being used instead of say a button for the file upload input selector? I think this does not follow a11y and even semantic html. I wouldn't think to click a label in a form to trigger the upload file popup. I'd try to do something like this

I'm not sure. Using label is a native way for associating with input, so don't need to call click on input when click on button. I think for a11y it is better using label instead of button in that case. Something like this (i may be completely wrong)

<div class="au-file-upload" id="js-drivers-license">
	<label class="au-label">Attach drivers license</label>
	<input class="au-file-upload__input" id="drivers-license" type="file" />
<!-- hide below when no js -->
	<label class="au-btn au-btn--secondary au-file-upload__label" for="drivers-license">Choose file</label> 
	<span class="No file chosen" />
</div>

2

Yes, thanks, i forgot about hiding elements.

3

Primary button there only for example :)
File list functionality may be various: may be we want just see the amount of files we selected like '3 files selected', or may be we want to manage uploaded files like in my PR, or use it as you say, or add an "upload" button on each item in list with upload progress bar, or all of that and switch between options :)

Unfortunately multiple file feature, accept and drag&drop is not supported in < IE10. You can inform user to choose file one by one and add an new input after user select file. It seem like completely separate package specific for that browsers. Also i think u can't access file data because FileAPI is not supported, so cant count size and etc.

4

It depends on functionality, it can be even better to have simple and small single select component and multiple select component with different modes, file list management, upload progress bars, other cool fireworks. And some "core" module between them.

Of course we could release an alpha version just need to decide features.

@azerella
Copy link
Contributor

azerella commented Mar 13, 2019

Branch rebase on latest develop now. Added email :)

@adamzerella Do i need to resolve conflicts in auds.json? Or i can simply accept all from develop and then run npm i in package directory?

@toxamiz I would accept the latest develop and then do: npm run watch in any package to generate the latest auds.json

@sukhrajghuman
Copy link
Contributor

sukhrajghuman commented Mar 13, 2019

@toxamiz thanks for getting back!

If you can find me an examples/research that supports using a label instead of a button for clicking to upload files, I'd be glad to accept this approach.

Yes lets split this PR up and focus on releasing a file upload component for a single file. I think that would provide good user value and is a lot more simple to build with fewer edge cases. What do you think?

@ghost
Copy link
Author

ghost commented Mar 14, 2019

@adamzerella OK, thanks

@sukhrajghuman

If you can find me an examples/research that supports using a label instead of a button for clicking to upload files, I'd be glad to accept this approach.

I don't think there are researches "button vs label on input file". If you want add a caption on form control better use label tag than any "label like" element.

According to this label for form control actually works like button providing a click events to associated form control. Almost any articles about custom styling file input uses styles on label. Buttons uses for other features.

Yes lets split this PR up and focus on releasing a file upload component for a single file. I think that would provide good user value and is a lot more simple to build with fewer edge cases. What do you think?

OK, lets focus on single file upload. So what do we need?

  • Default markup
  • Check file type according to accept attribute
  • File size restriction
  • Error handling/messages
  • ...

@sukhrajghuman
Copy link
Contributor

sukhrajghuman commented Mar 14, 2019

@toxamiz
Hmmm yea okay I know what you are saying, can we style the associated label look like a button (or link) then. So it doesn't look like this:

image

What do you think?

@sukhrajghuman
Copy link
Contributor

But yea sorry I probably didn't word myself properly. What I mean to say is that it's not intuitive to click on text to trigger an action. I know labels are used with a for-id relationship to focus on the form control, but you shouldn't rely on that, and you are assuming that when people fill out a form they will click the label. If something requires clicking it needs to be designed like so.

A picture from an article on affordance to better illustrate what I'm trying to say.

image

I would also check out:

https://design-system.service.gov.uk/components/file-upload/
https://home-office-digital-patterns.herokuapp.com/patterns/upload-file
alphagov/govuk-design-system-backlog#49

@ghost
Copy link
Author

ghost commented Mar 15, 2019

@sukhrajghuman
I see the reason of misunderstanding. Actually i meant that label should be look like a button, but for some reason didn't add an au-btn class. :) There is no doubt that if something requires clicking it needs to be designed like so.

I think the main accessibility issue with file upload is that native upload provide it's own button and text. I think the better way is without js it should both label and native input. Only with js it converts into button and visually hidden input + some dynamically added element showing which file uploaded. It's not enough to add some css classes for visual appearance

Something like this:
image

@sukhrajghuman
Copy link
Contributor

@toxamiz yes but that above example could potentially be confusing in the context of a form with a primary submit button. Hence why in my first example I used the au-btn--secondary.

image

It's not an easy problem. There are so many edge cases. I will have to talk with our accessibility team for more guidance.

Right now I'm thinking we just stick with the browser default file upload, and add some additional styling (fonts, spacing, errors, focus states etc.) That way we don't have to worry about javascript either.

@ghost
Copy link
Author

ghost commented Mar 18, 2019

I will have to talk with our accessibility team for more guidance.

That would be great.

Right now I'm thinking we just stick with the browser default file upload, and add some additional styling (fonts, spacing, errors, focus states etc.) That way we don't have to worry about javascript either.

Good idea. Is there any sketches?

@ghost ghost force-pushed the feature/file-upload branch from d005da1 to 62fcdf5 Compare March 19, 2019 09:23
@azerella azerella modified the milestones: Sprint 125, Sprint 126 Mar 20, 2019
@sukhrajghuman
Copy link
Contributor

Good idea. Is there any sketches?

Not at this stage sorry. I was basing this off the gov uk implementation of the file upload component, as mentioned in my previous post. Will keep you in the loop

@sukhrajghuman sukhrajghuman removed this from the Sprint 126 milestone Apr 3, 2019
@azerella azerella mentioned this pull request Apr 4, 2019
3 tasks
@azerella azerella changed the title [WIP] File upload component [DRAFT] File upload component May 1, 2019
Repository owner closed this by deleting the head repository Oct 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request. help wanted Extra attention is needed. in progress Currently working on. new component An issue or pull request related to a new component.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants