Fix/on before file added not called #1597
Conversation
| renderPoweredByUppy () { | ||
| return <a tabindex="-1" href="https://uppy.io" rel="noreferrer noopener" target="_blank" class="uppy-Dashboard-poweredBy"> | ||
| {this.props.i18n('poweredBy')} | ||
| <svg aria-hidden="true" class="UppyIcon uppy-Dashboard-poweredByIcon" width="11" height="11" viewBox="0 0 11 11"> |
There was a problem hiding this comment.
Spacing between “powered by” text and “Uppy” with icon is gone, adding something like {' '} should fix it, I think.
| } | ||
|
|
||
| renderAcquirer (acquirer) { | ||
| return <div class="uppy-DashboardTab" role="presentation"> |
There was a problem hiding this comment.
Nitpick: since you’ve refactored this (thanks!), could we add () after all of the return statements, maybe?
There was a problem hiding this comment.
@arturi, let's, but let's add eslint rules for these cases?
I also see both lowercase props (e.g. onpaste) and camelCase (onPaste) used interchangeably, that's something that's nice to have handled by eslint.
There was a problem hiding this comment.
Yes, good idea about eslint for those. But maybe in a separate PR after accessibility and Alex’s PRs are merged? Cause we’ll need to touch everything with strict rules.
| type="file" | ||
| name="files[]" | ||
| multiple={this.props.maxNumberOfFiles !== 1} | ||
| // Clear file input |
There was a problem hiding this comment.
Maybe add that setting an empty value="" wasn’t enough, and we are doing this to fix onBeforeFileAdded? Not sure, but might be helpful in the future?
There was a problem hiding this comment.
@arturi, agreed, I'll add it.
There was a problem hiding this comment.
Nice! Thanks. Left some minor comments.
Another issue is that this addresses Dashboard, while the issue is talking about FileInput, and DragDrop probably has the same issue? So we need this change across all 3 plugins, right?
If needed, we can merge the DragDrop PR first, if you anticipate conflicts there.
|
@arturi, good note, I could handle these:
Will you check /feature/accessibility today? |
|
Ideally we handle |
I think it's a small enough change for conflicts to not arise, I'll add it. |
|
Could you update the |
|
|
||
| // ___Why not use value="" on <input/> instead? | ||
| // Because if we use that method of clearing the input, Chrome will not trigger onChange={} if we drop the same file twice (Issue #768). | ||
| event.target.value = null |
There was a problem hiding this comment.
We pass ev to the callback, but event works anyway. What sort of sorcery is this? 🙀 Been trying to find documentation for a while, nothing so far.
There was a problem hiding this comment.
It's a (very old) global property: https://developer.mozilla.org/en-US/docs/Web/API/window/event
It allows you to refer to the event in an inline on* event handler attribute, eg <input onclick="event.target.value = undefined">
best not to rely on it :)
There was a problem hiding this comment.
Wow, thanks! Crazy. A thought about it being global occurred to me briefly when eslint didn’t complain.
Is it safe to call the callback argument event: (event) => { ... }? Thus overriding this global event.
There was a problem hiding this comment.
I didn't fucking notice! It wasn't intentional, I'm amazed it worked too.
Eslint usually catches undeclared variables, we seem to be having a very lenient configuration? I also noticed it doesn't tell me what class methods are not used.
There was a problem hiding this comment.
Is it safe to call the callback argument
event:(event) => { ... }? Thus overriding this globalevent.
yes
i'm also surprised that this isn't caught, I thought standard has a no-confusing-globals rule or something, may need to look into that
There was a problem hiding this comment.
Standard includes this http://eslint.org/docs/rules/no-undef, couldn’t figure out why that wouldn’t work yet.
There is another rule we could add, it’s talking about event specifically: https://eslint.org/docs/rules/no-restricted-globals
|
Pushed a change for FileInput, updated comments and changed Could you check that it works on your end again, please? |
|
Looks good, works for Dashboard, DragDrop and FileInput. |
Small refactoring of the
<AddFiles/>(to avoid repeating the fix for hidden inputs in multiple files),and fixes #768.