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

Convert ondragenter example to a live sample #8667

Merged
merged 12 commits into from
Sep 10, 2021
Merged

Conversation

AnilSeervi
Copy link
Contributor

@AnilSeervi AnilSeervi commented Sep 5, 2021

  • Add missing opening <head> tag
  • Rearrange closing </head> tag
  • Add quotes to value of lang attr

@AnilSeervi AnilSeervi requested a review from a team as a code owner September 5, 2021 15:37
@AnilSeervi AnilSeervi requested review from sideshowbarker and removed request for a team September 5, 2021 15:37
@github-actions
Copy link
Contributor

github-actions bot commented Sep 5, 2021

Preview URLs

Flaws

None! 🎉

External URLs

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

No new external URLs

(this comment was updated 2021-09-10 15:35:23.266600)

@wbamberg
Copy link
Collaborator

wbamberg commented Sep 5, 2021

Thanks for this PR!

Would you like to try converting it into a live sample?

It would involve:

  • splitting up that HTML code block into three parts: HTML, CSS, JavaScript, and placing each part under an H3 heading under the existing "Example" H2. The HTML part only needs to include the content of the <body>.

  • Adding a fourth H3, "Result" and under that adding a line like: {{EmbedLiveSample("Example", "100", "300""}} (you might want to experiment with the third parameter, which sets the iframe height).

I think this would make the page much more useful!

For the next level improvement, rather than using console.log() for output, the example might write the output into a part of the iframe (like a <div> underneath the drop zone, say) - then readers would be able to see the output without needing the devtools open.

@AnilSeervi
Copy link
Contributor Author

Thanks for this PR!

Would you like to try converting it into a live sample? This would make the page much more useful IMO.

It would involve:

  • splitting up that HTML code block into three parts: HTML, CSS, JavaScript, and placing each part under an H3 heading under the existing "Example" H2. The HTML part only needs to include the content of the <body>.

  • Adding a fourth H3, "Result" and under that adding a line like: {{EmbedLiveSample("Example", "100", "300""}} (you might want to experiment with the third parameter, which sets the iframe height).

I think this would make the page much more useful!

For the next level improvement, rather than using console.log() for output, the example might write the output into a part of the iframe (like a <div> underneath the drop zone, say) - then readers would be able to see the output without needing the devtools open.

Will try converting it to the liveSample.

@wbamberg
Copy link
Collaborator

wbamberg commented Sep 5, 2021

Oh, I forgot. Here's an example of a page that contains a live sample: https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/progress_event#live_example . This one doesn't include CSS, and personally I would use "JavaScript" as the heading text rather than "JS", but otherwise it's a reasonable one to copy :).

@wbamberg
Copy link
Collaborator

wbamberg commented Sep 5, 2021

@AnilSeervi
Copy link
Contributor Author

Thank you. I'll attempt converting the example to livesample for this page first and then have the same replaced in other pages you've mentioned accordingly.

@AnilSeervi
Copy link
Contributor Author

Also, predictably, all the related events use similar examples:

https://developer.mozilla.org/en-US/docs/Web/API/GlobalEventHandlers/ondrag
https://developer.mozilla.org/en-US/docs/Web/API/GlobalEventHandlers/ondragend
https://developer.mozilla.org/en-US/docs/Web/API/GlobalEventHandlers/ondragleave
https://developer.mozilla.org/en-US/docs/Web/API/GlobalEventHandlers/ondragover
https://developer.mozilla.org/en-US/docs/Web/API/GlobalEventHandlers/ondragstart
https://developer.mozilla.org/en-US/docs/Web/API/GlobalEventHandlers/ondrop

These should all be live samples. But if this is too big a task for you now, let me know and I'll file a bug. Or file one yourself if you prefer :).

You may file the bug and assign it to me. I'll work on it.

@wbamberg
Copy link
Collaborator

wbamberg commented Sep 5, 2021

Thank you. I'll attempt converting the example to livesample for this page first and then have the same replaced in other pages you've mentioned accordingly.

That sounds like a great plan :).

@AnilSeervi
Copy link
Contributor Author

@wbamberg, does this look good?

Copy link
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

Thanks @AnilSeervi , this is so much better!

I had some code style suggestions, please ask if any of them don't make sense to you :).

@wbamberg
Copy link
Collaborator

@AnilSeervi , please let me know if you think you will get to this in the next couple of days. I'm sorry to nag, but I'm planning to convert all the Web/API docs into Markdown early next week, and would like to close as many PRs as possible beforehand. If you won't have time no worries - I could finish it myself.

@AnilSeervi
Copy link
Contributor Author

@AnilSeervi , please let me know if you think you will get to this in the next couple of days. I'm sorry to nag, but I'm planning to convert all the Web/API docs into Markdown early next week, and would like to close as many PRs as possible beforehand. If you won't have time no worries - I could finish it myself.

Sorry, I got busy. I'll make the changes right now.

@AnilSeervi
Copy link
Contributor Author

AnilSeervi commented Sep 10, 2021

I'm yet to add a reload/restart button to revert the drag. I was thinking, is there a way to just reload/refresh the iframe so that the example will be in its initial state again?

Edit: Didn't quite notice that you already suggested it here #8667 (comment)

Copy link
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

@AnilSeervi thanks for all your work on this! It's a big improvement to the page.

@wbamberg wbamberg merged commit 38ce7ee into mdn:main Sep 10, 2021
@AnilSeervi
Copy link
Contributor Author

AnilSeervi commented Sep 10, 2021

No problem.
What about the other pages that use the same but slightly altered examples. Should they also be changed to live samples before conversion to markdown?.

@wbamberg
Copy link
Collaborator

They don't need to be fixed before the conversion, no. We can do it afterwards.

What we really want is to minimise the number of PRs against Web/API that are open at the time we convert (hopefully this Tuesday: #8741), as they will have to be redone.

@AnilSeervi
Copy link
Contributor Author

They don't need to be fixed before the conversion, no. We can do it afterwards.

What we really want is to minimise the number of PRs against Web/API that are open at the time we convert (hopefully this Tuesday: #8741), as they will have to be redone.

Sure. Will work on it after they are converted.

I should have changed the PR title before it was merged.

@wbamberg wbamberg changed the title add missing <head> tag and rearrange closing tag Convert ondragenter example to a live sample Sep 10, 2021
@AnilSeervi AnilSeervi deleted the patch-1 branch September 12, 2021 09:09
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants