Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix/on before file added not called #1597

Merged
merged 5 commits into from
May 31, 2019
Merged

Fix/on before file added not called #1597

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1877113
@uppy/dashboard - mild refactor of <AddFiles/>
lakesare May 24, 2019
f315325
@uppy/dashboard - made file input clear even when same file is chosen…
lakesare May 24, 2019
b871c05
@uppy/Dashboard - formatted <AddFiles/> as per eslint
lakesare May 27, 2019
c52f35d
@uppy/drag-drop - made .onBeforeFileAdded() be called twice even if f…
lakesare May 27, 2019
9926853
Clear input[type=file] in FileInput too, updated comments and changed…
arturi May 30, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions packages/@uppy/drag-drop/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,10 @@ module.exports = class DragDrop extends Plugin {
// Nothing, restriction errors handled in Core
}
})

// ___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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@goto-bus-stop goto-bus-stop May 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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 :)

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@lakesare lakesare May 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it safe to call the callback argument event: (event) => { ... }? Thus overriding this global event.

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

render (state) {
Expand Down Expand Up @@ -140,11 +144,7 @@ module.exports = class DragDrop extends Plugin {
name={this.opts.inputName}
multiple={restrictions.maxNumberOfFiles !== 1}
accept={restrictions.allowedFileTypes}
ref={(input) => {
this.input = input
}}
onchange={this.handleInputChange}
value="" />
onchange={this.handleInputChange} />
{this.i18nArray('dropHereOr', {
browse: <span class="uppy-DragDrop-dragText">{this.i18n('browse')}</span>
})}
Expand Down