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

Make EditContext demo match the MDN tutorial (remove useless setInterval, debug mode, reorganize in files, clean up code) #266

Merged
merged 5 commits into from
Mar 18, 2024
Merged

Make EditContext demo match the MDN tutorial (remove useless setInterval, debug mode, reorganize in files, clean up code) #266

merged 5 commits into from
Mar 18, 2024

Conversation

captainbrosset
Copy link
Contributor

@captainbrosset captainbrosset commented Mar 18, 2024 •
edited
Loading

This is to make the EditContext API demo match the tutorial being written in mdn/content#32027.

  • Removal of setInterval, which makes it hard to debug the demo with DevTools.
  • Removal of the debug mode helped me during development, but is making the demo code harder to follow now.
  • Split the demo into multiple files.
  • Removed some code ambiguity.
  • General refactoring and cleanup.

@captainbrosset captainbrosset requested a review from a team as a code owner March 18, 2024 09:37
@captainbrosset captainbrosset requested review from bsmth and removed request for a team March 18, 2024 09:37
@captainbrosset captainbrosset changed the title Remove a useless setInterval-based render in the EditContext demo Remove a useless setInterval-based render and debug mode in the EditContext demo Mar 18, 2024
@captainbrosset captainbrosset changed the title Remove a useless setInterval-based render and debug mode in the EditContext demo Remove useless setInterval-based render and debug mode, and reorganize the EditContext demo Mar 18, 2024
@bsmth
Copy link
Member

bsmth commented Mar 18, 2024

Taking a look now 👀

@bsmth
Copy link
Member

bsmth commented Mar 18, 2024

BTW, the guide is 404ing: https://github.com/mdn/dom-examples/tree/main/edit-context#demos

@captainbrosset captainbrosset marked this pull request as draft March 18, 2024 10:23
@captainbrosset
Copy link
Contributor Author

Sorry @bsmth , I may need to do a couple more changes. Please hold off.

bsmth
bsmth reviewed Mar 18, 2024
View reviewed changes
Copy link
Member

@bsmth bsmth left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks! I took a look through the diffs and tested using:

cd edit-context/html-editor
http-server -c-1

Also compared with the deployed example and looks good.

  • Not working on FX (as expected)
  • Works as expected in Chrome Version 122.0.6261.129

@captainbrosset
Copy link
Contributor Author

BTW, the guide is 404ing: https://github.com/mdn/dom-examples/tree/main/edit-context#demos

I'm working on it now in this PR mdn/content#32027
Hope it's ok to leave the link in for now, as I expect to be able to merge that PR in the not too distant future.

@bsmth
Copy link
Member

bsmth commented Mar 18, 2024

Sorry @bsmth , I may need to do a couple more changes. Please hold off.

Oops, I was too fast. You can ping me again when you're ready

@captainbrosset
Copy link
Contributor Author

captainbrosset commented Mar 18, 2024 •
edited
Loading

Oops, I was too fast. You can ping me again when you're ready

No it's my fault! I thought it would be just the one initial change, but then ended up doing more changes. I should have switched it to draft earlier. Thanks for the time spent on this already! I'll ping you again as soon as I'm done.

@captainbrosset captainbrosset marked this pull request as ready for review March 18, 2024 13:55
@captainbrosset captainbrosset changed the title Remove useless setInterval-based render and debug mode, and reorganize the EditContext demo Make EditContext demo match the MDN tutorial (remove useless setInterval, debug mode, reorganize in files, clean up code) Mar 18, 2024
@captainbrosset
Copy link
Contributor Author

@bsmth I'm done with my changes. Over to you for review, whenever you have some time.

@bsmth
Copy link
Member

bsmth commented Mar 18, 2024

Having another look now 👀

bsmth
bsmth approved these changes Mar 18, 2024
View reviewed changes
Copy link
Member

@bsmth bsmth left a comment

Choose a reason for hiding this comment

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

Thanks a lot, mostly just reviewed c0818e3 since the last approval. Tested in Chrome again and looking good!

Thank you! 👍🏻

@captainbrosset
Copy link
Contributor Author

Awesome! Feel free to merge whenever.

@bsmth bsmth merged commit 9098959 into mdn:main Mar 18, 2024
3 checks passed
@captainbrosset captainbrosset deleted the remove-setinterval branch March 19, 2024 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bsmth bsmth bsmth approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@captainbrosset @bsmth