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
Named exports from ol/has #7764
Conversation
Hey Tim, looking into this now. Please see #7765 |
Thanks @marcjansen. I'm debugging the test failure with dashed lines. It looks like we shouldn't have any conflicts between this and #7765. I'll review that after fixing up the tests here. |
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.
Code looks ok. I have added some questions and think we need to come up with something for canvas line dashes.
The remaining failing tests seem to mock DEVICE_PIXEL_RATIO
and might need changes as well.
src/ol/has.js
Outdated
return false; | ||
} | ||
})(); | ||
export const CANVAS_LINE_DASH = false; |
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.
That is quite a change or not?
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.
just read your reply above.
}); | ||
|
||
it('works in DOM_DELTA_LINE mode (wheel)', function(done) { | ||
if (FIREFOX) { |
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 change is OK for me.
it('works on Safari (wheel)', function(done) { | ||
const origHasSafari = _ol_has_.SAFARI; | ||
_ol_has_.SAFARI = true; | ||
if (!FIREFOX) { |
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.
Isn't this too broad? Should this test for Safari explicitly?
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 diff view may be making it hard to see, but this condition wraps a test that used to set FIREFOX = false
. See https://github.com/openlayers/openlayers/pull/7764/files#diff-7c3aa34c433b5b7112b6f12d7aeea7aaL86.
@marcjansen - I've addressed the previously failing test about line dashes by properly setting |
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.
LGTM. Thanks!
This reworks the
ol/has
module to use named exports.