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.

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

fix(ui5-upload-collection): fire drop only when files are dropped within dnd overlay #2527

Merged
merged 2 commits into from
Nov 30, 2020
Merged

fix(ui5-upload-collection): fire drop only when files are dropped within dnd overlay #2527

merged 2 commits into from
Nov 30, 2020

Conversation

georgimkv
Copy link
Contributor

Fixes: #2001

@georgimkv georgimkv requested a review from a team November 30, 2020 11:07
@CLAassistant
Copy link

CLAassistant commented Nov 30, 2020

CLA assistant check
All committers have signed the CLA.

@@ -712,7 +712,7 @@ class UI5Element extends HTMLElement {
// This will be false if the normal event is prevented
const normalEventResult = this.dispatchEvent(customEvent);

// Return false if any of the two events was prevented (its result was false).
// Return false if any of the two events were prevented (its result was false).
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure it must be "were" since "any" is the subject in the sentence (and not "events") and it is singular. One would say "return false if any event was prevented", hence "return false if any of the events was prevented" (and "any" is still the subject). Anyway, not important, just curious :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I read a bit more about this. "Any of" here is plural. Turns out, both forms of the verb are accepted. Use of the singular form is more "formal", plural is more "informal". I'll revert it back since this isn't a grammar error as I thought. See 2. 'any of' here.

@@ -125,6 +125,16 @@ const metadata = {
},
},
events: /** @lends sap.ui.webcomponents.fiori.UploadCollection.prototype */ {
/**
* Fired when an element is dropped inside the drag and drop overlay.
Copy link
Contributor

Choose a reason for hiding this comment

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

Normally we don't document browser events, because they are fired and work anyway. However, we do this for browser events that we modify (such as this one) in order to explain what is different than the expected normal behavior. What you described here is correct, but it would have been correct before the change too. So I'd suggest adding:

<b>Note:</b> The <code>drop</code> event is fired only for the drag and drop overlay and ignored for the other parts of the <code>ui5-upload-collection</code>

or something to this effect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@vladitasev vladitasev merged commit 455f614 into SAP:master Nov 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[All] Dropping a file outside upload collection removes page scrollbar
3 participants