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

ondragstart: convert static example to live example #15318

Merged
merged 4 commits into from
Apr 29, 2022

Conversation

AnilSeervi
Copy link
Contributor

Summary

ondragstart: convert static example to live example

Motivation

Open-source

Supporting details

Related issues

fixes portion of #11364

Metadata

  • Adds a new document
  • Rewrites (or significantly expands) a document
  • Fixes a typo, bug, or other error

@AnilSeervi AnilSeervi requested a review from a team as a code owner April 24, 2022 13:30
@AnilSeervi AnilSeervi requested review from sideshowbarker and removed request for a team April 24, 2022 13:30
@github-actions github-actions bot added the Content:WebAPI Web API docs label Apr 24, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Apr 24, 2022

Preview URLs

Flaws

URL: /en-US/docs/Web/API/GlobalEventHandlers/ondragstart
Title: GlobalEventHandlers.ondragstart
on GitHub
Flaw count: 2

  • macros:
    • wrong xref macro used (consider changing which macro you use)
    • wrong xref macro used (consider changing which macro you use)

External URLs

URL: /en-US/docs/Web/API/GlobalEventHandlers/ondragstart
Title: GlobalEventHandlers.ondragstart
on GitHub

No new external URLs

(this comment was updated 2022-04-27 06:11:51.862194)

@@ -16,7 +16,7 @@ A {{domxref("GlobalEventHandlers","global event handler")}} for the
## Syntax

```js
var dragstartHandler = targetElement.ondragstart;
targetElement.ondragstart = dragstartHandler;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks @AnilSeervi but I would literally copy the structure, using value rather than return type - so it would look like this

image

@hamishwillee
Copy link
Collaborator

Excellent! I'd fix up the syntax section but otherwise this is much better.

@sideshowbarker sideshowbarker removed their request for review April 24, 2022 23:16
AnilSeervi and others added 2 commits April 25, 2022 07:24
Co-authored-by: Hamish Willee <hamishwillee@gmail.com>
@AnilSeervi AnilSeervi requested a review from hamishwillee April 25, 2022 01:57
Select this element, drag it to the Drop Zone and then release the selection to move the element.
</p>
</div>
<div id="target">Drop Zone</div>
Copy link
Contributor

@OnkarRuikar OnkarRuikar Apr 25, 2022

Choose a reason for hiding this comment

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

Suggested change
<div id="target">Drop Zone</div>
<div id="destination">Drop Zone</div>

As we are on an event page, the 'target' and 'event.target' can become source of confusion.
Also, in syntax section we used target.ondragstart but in code we say source.ondragstart. Are we calling draggable elements 'target' or 'source'? In the code we can not use target.ondragstart cos we are calling destination "target" here.


// Set handlers for the source's drag - start/enter/leave/end events
source.ondragstart = dragstart_handler;
source.ondragenter = dragenter_handler;
Copy link
Contributor

@OnkarRuikar OnkarRuikar Apr 25, 2022

Choose a reason for hiding this comment

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

Suggested change
source.ondragenter = dragenter_handler;
target.ondragenter = dragenter_handler;

I think these(also ondragleave) should be registered on drop target[ref]. On drag leave the target element is not resetting to white color from lightblue. There is no need to indicate that the draggable is on source element itself.

@OnkarRuikar
Copy link
Contributor

@hamishwillee and @teoli2003, are we treating these pages as event pages or as property pages(cos they look like a property element.onXYZHandler).

Which tags are applicable to this page or such pages?

  • Property
  • Event
  • GlobalEventHandlers
  • Event Handler

@hamishwillee
Copy link
Collaborator

@OnkarRuikar I think all of these should "eventually" be event pages. However as the fix was to an old-style property page, I commented on the fix in that context. We certainly shouldn't block the fix here on converting to something else.

@OnkarRuikar
Copy link
Contributor

@OnkarRuikar I think all of these should "eventually" be event pages. However as the fix was to an old-style property page, I commented on the fix in that context. We certainly shouldn't block the fix here on converting to something else.

I agree. Sorry for the miscommunication. I asked this in context of updating all the pages to modern structure. Like it's being done in #15335 (Update Method pages to modern structure) PR series. Once we fix the structure it can be done in one go.


Regarding this PR I think we need to put ondragenter and ondragleave handlers on the destination element(target) and not on source element. By putting it on source element we are indicating(with yellow color) that the source element is being dragged over itself. Unless that is the intention. If we look at the example in dragenter_event#examples the JSFiddle code handles it for all the containers.

Also, there is a bug that the destination element doesn't change back to original color(from lightblue) when dragged item leaves the destination element. target.ondragover is not the right event to turn destination color to blue as it keeps on firing while the draggable is over the target.

@Elchi3
Copy link
Member

Elchi3 commented Apr 25, 2022

fwiw, I'm still working on event pages. Last week I finished WindowEventHandlers and GlobalEventHandlers is next on my list. The project is described here: openwebdocs/project#61 and I reported about how far we've come so far here: https://github.com/openwebdocs/project/blob/main/impact-report-q1-2022/impact-report-q1-2022.md#rewriting-reference-pages-for-all-web-platform-events

Updates to the example sections are great, so I appreciate this PR. Structurally we agreed that on- pages will be removed and we will just have eventname_event pages, so you maybe it is not worth investing into structural updates in this PR.

@hamishwillee
Copy link
Collaborator

Thanks all.

@AnilSeervi So the direction to you is to look at this suggestion by Onkar #15318 (comment) starting with "Regarding this PR I think we need to put ondragenter and ondragleave handlers on the destination element(target) and ..."

If/once that is OK we'll ignore any other issues and get this in.

Copy link
Contributor Author

@AnilSeervi AnilSeervi left a comment

Choose a reason for hiding this comment

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

I think the colors and events are applied correctly now.

Copy link
Collaborator

@hamishwillee hamishwillee left a comment

Choose a reason for hiding this comment

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

@OnkarRuikar This looks good to me, but probably best if you review the changes you requested ....

@OnkarRuikar
Copy link
Contributor

OnkarRuikar commented Apr 29, 2022

@hamishwillee Looks good to me too.

@hamishwillee
Copy link
Collaborator

Thanks @AnilSeervi - much much better :-)

@hamishwillee hamishwillee merged commit 49a92b9 into mdn:main Apr 29, 2022
@AnilSeervi AnilSeervi deleted the convert-ondragstart-live-example branch April 29, 2022 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:WebAPI Web API docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants