-
Notifications
You must be signed in to change notification settings - Fork 28
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
volume control fix on scroll #430
Conversation
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 great! One suggestion.. thank you!
scrollTop = document.body.scrollTop; | ||
scrollLeft = document.body.scrollLeft; | ||
const supportPageOffset = window.pageXOffset !== undefined; | ||
const isCSS1Compat = (document.compatMode || '') === 'CSS1Compat'; |
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.
I think we can skip "isCSS1Compat" so get rid of all the stuff with document.documentElement.
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.
the problem is that document.documentElement is supported for Ie: https://developer.mozilla.org/en-US/docs/Web/API/Document/documentElement
so I think we might need it.
this is another suggestion:
const supportPageOffset = window.pageXOffset !== undefined;
const BackCompat = document.compatMode === 'BackCompat';
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.
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.
Ok i removed document.documentElement :-) I can confirm that everything is still working
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.
Great thank you!!
I will try to make a new release tonight.. remind me if I forget. Thanks again! |
@benwiley4000 just a reminder for the release :-) |
Sorry for the radio silence. I've had a crazy week but I really will try to
do this tonight. Setting a reminder.
Ben
Le ven. 20 sept. 2019 17 h 28, Robin Giel <[email protected]> a
écrit :
… @benwiley4000 <https://github.com/benwiley4000> just a reminder for the
release :-)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#430?email_source=notifications&email_token=ADHOD3L27K4AVUGUVCCJ4W3QKU577A5CNFSM4IXHU6BKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7H5M2Q#issuecomment-533714538>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADHOD3OIYMWL4P4WSGYYCULQKU577ANCNFSM4IXHU6BA>
.
|
Does this close #407? |
@RobinGiel I just published v2.0.0-beta.4! Thanks. And sorry again for the wait. |
According to MDN: For cross-browser compatibility, use window.pageYOffset instead of window.scrollY. Additionally, older versions of Internet Explorer (< 9) do not support either property and must be worked around by checking other non-standard properties. A fully compatible example:
`var supportPageOffset = window.pageXOffset !== undefined;
var isCSS1Compat = ((document.compatMode || "") === "CSS1Compat");
var x = supportPageOffset ? window.pageXOffset : isCSS1Compat ? document.documentElement.scrollLeft : document.body.scrollLeft;
var y = supportPageOffset ? window.pageYOffset : isCSS1Compat ? document.documentElement.scrollTop : document.body.scrollTop; `
https://developer.mozilla.org/en-US/docs/Web/API/Window/scrollY#Notes
Tested on Firefox, Chrome, Safari, Edge and Internet Explore