-
Notifications
You must be signed in to change notification settings - Fork 22.5k
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
Hint to use addEventListener
instead of the onchange property
#28105
Conversation
The onchange is a global and if multiple libraries use it, they will override each other, it's better to recommend the use of `addEventListener` (which is not documented on the linked page)
…nstead of just `onchange`
Preview URLs
(comment last updated: 2023-07-24 21:35:13) |
That commit had changed all the line ending in the file to Windows CRLF rather than just LF. The substantive part of the change was this: -/en-US/docs/Web/API/ScreenOrientation/onchange /en-US/docs/Web/API/ScreenOrientation
+/en-US/docs/Web/API/ScreenOrientation/onchange /en-US/docs/Web/API/ScreenOrientation#events …so I backed out the CRLF change and amended and force-pushed the commit to just include that change. |
I was using the github UI for those changes so it's a bit weird it changed all the line endings in the txt file but not the md files Thanks for fixing the commit |
Thanks for the PR, @Tofandel , and Mike for fixing the redirects file. It's the weekend here but I will look at this PR on Monday. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your PR, @Tofandel . You're right that there's work we need to do here but this isn't quite it.
We don't give the onxyz
properties their own pages on MDN any more, but we do have pages for the events, which have URLs like xyz_event
and titles like xyz
, and we list these pages under the "Events" heading in the interface page.
See https://developer.mozilla.org/en-US/docs/Web/API/Animation#events for an example.
So we need a new page at https://developer.mozilla.org/en-US/docs/Web/API/ScreenOrientation/change_event, that describes the event, following a roughly similar pattern to https://developer.mozilla.org/en-US/docs/Web/API/Animation/finish_event (see also the template at https://developer.mozilla.org/en-US/docs/MDN/Writing_guidelines/Page_structures/Page_types/API_event_subpage_template).
Then we can link to that page from the entry at https://pr28105.content.dev.mdn.mozit.cloud/en-US/docs/Web/API/ScreenOrientation#events.
Please let me know if you want to make this change. If you don't I'm happy to close this PR have make the change myself.
@wbamberg This is what I noticed when I copied the events presentation from another api page, but adding a sub page was a bit out of my depth I added it but I'm not sure why this headers are showing, because I don't think it is in fact experimental and secure context |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pretty great! I had a few comments, mostly copy-edits.
Co-authored-by: wbamberg <[email protected]>
Co-authored-by: wbamberg <[email protected]>
Co-authored-by: wbamberg <[email protected]>
Co-authored-by: wbamberg <[email protected]>
Co-authored-by: wbamberg <[email protected]>
Co-authored-by: wbamberg <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 looks awesome, thank you for your contribution, @Tofandel !
Description
The onchange is a global and if multiple libraries use it, they will override each other's listener, it's better to recommend the use of
addEventListener
which I also documented on the linked page instead ofscreen.orientation.onchange
Motivation
I could not find a proper replacement when looking at the deprecated event and the linked page, because it only proposes a global
onchange
, I had to google it and find this stackoverflow answer to find thatScreenOrientation
supports the use ofaddEventListener