Add guidance about HTML attributes to File Upload component#5248
Conversation
✅ You can preview this change here:
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@calvin-lau-sig7 and @seaemsi, for your awareness. See the preview for this particular new section. |
|
Hi @selfthinker, thanks for the heads-up! Looking at this as a small change, so given everything going on I'm fine too to keep it minimal. The draft reads really well :). I've made a suggestion to align with the style guide on contractions and bullet points. |
3050a36 to
01053fc
Compare
Thanks. That might be because I ran it over Hemingway before. :)
Thanks. One day I'll remember that rule about contractions. As you can see from my original draft, I had initially put the options into bullet points and the removed them in order to shorten the section. @calvin-lau-sig7, thanks for reviewing and correcting. I have now made the changes. |
01053fc to
c500c41
Compare
|
|
||
| ### About HTML attributes | ||
|
|
||
| Some of the HTML attributes do not work as expected. |
There was a problem hiding this comment.
I think this needs to be more specific. In what context do they not work as expected? When passed to the attributes Nunjucks parameter?
There was a problem hiding this comment.
I tried to write this intentionally in a way so that it can be applied for both parameters in the HTML version as well as the Nunjucks version. Obviously, they might be called something different in the Nunjucks version, but I think it's still clear which ones they are?
There was a problem hiding this comment.
It still reads as weirdly ambiguous to me. The HTML version arguably makes that issue worse, as there are multiple elements that the statement could relate to (the input and the wrapper).
"Some HTML attributes applied to the input will not work the same way when using the improved File upload" maybe?
There was a problem hiding this comment.
Something like this maybe?
Some HTML attributes applied to the input might not work or work differently.
There was a problem hiding this comment.
Yeah, let's roll with that.
|
|
||
| Some attributes work on the original input, such as: | ||
|
|
||
| - `name` |
There was a problem hiding this comment.
Not sure name needs to be mentioned as this is a separate parameter already, it's not one that a user would need to pass in via attributes.
There was a problem hiding this comment.
I think it's important to clarify which parameters work where. If the name was on the button, it wouldn't work.
This is indeed less important for Nunjucks users, as it is set automatically and is required. But I don't think it would harm the Nunjucks users or would make them use it wrong.
I would ideally not want to write two different pieces of guidance, one for implementers who use the HTML version and one for Nunjucks users.
There was a problem hiding this comment.
Fair, though neither Nunjucks or HTML users have a means of passing a name to the button as it's entirely generated in JS.
There was a problem hiding this comment.
It's mostly there to say, "everything is fine with name, please don't even try to move it elsewhere".
|
|
||
| Some of the HTML attributes do not work as expected. | ||
|
|
||
| The JavaScript that creates this version only copies a few of the attributes from the original input. These are: |
There was a problem hiding this comment.
I think the wording here could use tweaking to be more direct (e.g. "The improved File upload will move some attributes from the native input to the improved component.")
That isn't really accurate though, as disabled and multiple aren't being moved or copied anywhere, they are still on the input. Only aria-describedby and id are moved.
There was a problem hiding this comment.
It's quite difficult to talk about these as we don't have a clear language for these things. Which of the elements are the component? When you suggest "The improved File upload will move some attributes from the native input to the improved component.", I would argue that is incorrect, as the attributes are not moved to the improved component, they already are in the improved component, just on the hidden input.
disabled is copied. It's both on the input and on the button.
Yes, multiple is not entirely accurate. multiples behaviour is copied over not the actual attributes. I had mentioned that in my description. I thought sacrificing accuracy for the sake of brevity would be worth it.
If you check my previous draft with more detail that I copied into the description, do you think any of that is worth keeping?
There was a problem hiding this comment.
Yeah, the lack of well defined language is annoying. Maybe just describing what is happening, i.e. "The improved File upload will move some attributes from the native input to the button." works better.
Maybe updating the bullet list to describe what happens is a good idea too. e.g.
- The value of the
idattribute is moved to the button and the input receives a new ID based on the old one. - The
disabledattribute is copied to the button and syncronised with any changes that happen on the input. - The
aria-describedbyattribute is moved to the button.
I'm not sure that having multiple listed is useful, especially if the proceeding paragraph is implying that it won't work as expected, when it actually does.
There was a problem hiding this comment.
What do you think of:
This version of the component will move the following attributes from the native input to the button:
Although I intentionally moved away from calling it "the input" and "the button" because most people would probably not understand what that means in this context. That's why I wrote "The JavaScript that creates this version" as that explains a bit of that.
And the reason why I called the input "the original input" was because that makes the relationship a bit clearer without necessarily needing to understand what is what in the code.
Regarding the bullet list, what do you think of my original draft?
What is moved or copied to the button:
idis added to the button and renamed on the inputdisabledis copiedmultipleis not directly copied but the behaviour of the button is adjusted accordinglyaria-describedbyis copied
Regarding multiple...
I'm not sure that having
multiplelisted is useful, especially if the proceeding paragraph is implying that it won't work as expected, when it actually does.
I think it is a bit expected. It works very similarly to the native input but a bit differently.
But the main point is that users need to be aware that if you want to have that attribute, it needs to be in the input and not on the button or container.
(I cannot easily be added to the button, but people might think that is where it belongs, as that is the visible thing. And they might try to put it in via JS somehow.)
There was a problem hiding this comment.
Would it be more helpful to phrase the lists as: These attributes work on the input (no matter how) and these others don't?
There was a problem hiding this comment.
What do you think of this simplification that combines both lists:
The following attributes work when added to the input:
iddisabledmultiplearia-describedbynameacceptcaptureaccesskey
(Although probably sorted alphabetically or something.)
There was a problem hiding this comment.
I think the original draft describing what changes is clearer than only listing the attribute names, but would still prefer if it were more technically accurate regarding what is moved vs. what is copied (id is copied, aria-describedby is moved, disabled is updated automatically based on the input state).
I guess the contention with multiple is that the paragraph before implies that we're doing something special to change how it works or where it's applied, which we aren't doing.
Maybe it should go into a new paragraph that mentions it alongside other file input-specific attributes? e.g.
HTML attributes that affect how a user can select files, such as
multiple,acceptandcapture, should still be added to theinputelement.
There was a problem hiding this comment.
Just to give an update to people not directly involved: I had made a document which shows three different levels of detail to ask the other developers (as the main target audience) what they prefer. The winner was the most detailed, so I'm going to rewrite this accordingly.
c500c41 to
1f7ecc5
Compare
There was a problem hiding this comment.
Still personally iffy on the inclusion of multiple and the implication that we're altering how it works somehow. It feels more accurate to put it in the second list with name, accept, et al.
I'm not going to let that block things, though. Happy with everything else.
My main reasoning for keeping it in the first list is because the JavaScript does something with it, while it doesn't do anything with the attributes in the second list. I hope that the way I have rewritten the intro for the first list makes it better. I'd be happy with changing how |
Thinking about it more, I think I might be agreeing with you. 🤔 |
1f7ecc5 to
38bf1a3
Compare
|
@calvin-lau-sig7, as this text has changed quite a bit since you last looked at it, can you do another content review? |
I've done that now, moved |
calvin-lau-sig7
left a comment
There was a problem hiding this comment.
Okay, reviewed!
Explaining the behaviour of elements, and JavaScript happening to them is a bit tricky.
38bf1a3 to
8eed136
Compare
calvin-lau-sig7
left a comment
There was a problem hiding this comment.
Looks good to me, the attributes explanation is read well!
|
Thanks both for reviewing! (@querkmachine, FYI, we have paired on fiddling with the words a bit more.) |
8eed136 to
b9f6bb2
Compare
|
On Friday we were mostly happy with the content but weren't sure if the lead in texts to the bullet lists convey things properly. I had an idea over the weekend and just made a final change. |
Co-authored-by: Calvin Lau <77630796+calvin-lau-sig7@users.noreply.github.com>
ffdf561 to
8acfc33
Compare
|
Thanks for the changes @calvin-lau-sig7, that all make sense. I've just merged them and plan to publish this within the next hour. |
This adds information about which HTML attributes work on the improved File Upload component and which don't.
This is to partially fix #5231.
We have to decide if we are fine with publishing this now or if it has to go together with the rest of the File Upload improvements (which will be published by the end of the next cycle, so in roughly 5 weeks).
I had initially written much more text but have condensed it to this version which is also intentionally a bit vague and even a little bit incorrect, as I'd otherwise have to write much more.
It's also worth nothing that this is written to be agnostic to people using the HTML version or the Nunjucks version of the component. It should apply to both.
This is what I originally wrote with more detail...
About HTML attributes
When the JavaScript creates the improved version of the component, it takes some of the attributes from the hidden
<input type="file">and adds them to the visible<button>, and leaves others on the hidden<input type="file">.What is moved or copied to the button:
idis added to the button and renamed on the inputdisabledis copiedmultipleis not directly copied but the behaviour of the button is adjusted accordinglyaria-describedbyis copiedThe rest is not moved or copied.
Some attributes only work on the hidden
<input type="file">while others would only work on the visible<button>, and one attribute works on neither.What attributes work on the hidden input:
nameacceptcaptureaccesskeyMost other attributes would need to be moved/copied/added to the button or the container
<div>, likedata-*attributes for tracking.Only the
requiredattribute works differently. In the improved version of the component it will not be read out to screen reader users, neither on the hidden input nor on the visible button. But native browser form validation will still work when put on the hidden input. Although we recommend not to use that attribute anyway.