-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Expose enabled and active states on interaction handlers #2238
Conversation
@@ -21,6 +23,9 @@ function BoxZoomHandler(map) { | |||
|
|||
BoxZoomHandler.prototype = { | |||
|
|||
enabled: false, | |||
active: 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.
Are you using active
anywhere?
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.
Some of the handlers, such as BoxZoomHandler
, already had a public (no leading underscore), albeit non-documented, active
property on them. I just took the opportunity to make the code, behaviour and documentation consistent with the new enabled
property.
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.
Got it! Thanks 😄
Looks great! Let's give @bhousel a chance to look this over and then we can merge it. |
Btw. @lucaswoj. There was a discussion on #2085 related to this. Obviously now that the handler has knowledge of whether it in fact is enabled or not the fallback of always disabling a handler before enabling it could be removed in favour of simply making Let me know if you prefer this refactoring instead of the current fallback behaviour. |
A problem with the previous code, but It's necessary for some handlers to know which other handlers are active, but no code outside the handler should be allowed to modify that state. |
@bhousel That's true, and obviously an inherent problem with mutable properties. With the same reasoning I take it you are for keeping the |
Next Steps
Anything else we should add to this list @bhousel? |
@lucaswoj The reason I went down the path of public properties in favour of accessors was @mourner's decision in #2173. Of course we can change that in accordance with your outline. Your suggested changes will be breaking, but I highly doubt anybody is currently using the non-documented In regards to your TODO list I'd love a comment about whether to keep the fallback |
My outline isn't cannon. I'm always open to discussion about the right way to do things. 😄 I'm not afraid of breaking changes to undocumented properties, as long as we're changing it to something we're comfortable documenting and supporting in the long haul. (Ah, the perks of being < 1.0.0 in semver).
I don't have a strong preference. Whatever you think best. |
@lucaswoj np. I'll provide a new set of commits... |
Thank you @averas! I'm going to test all the interaction handlers by hand and then I feel good about 🚢 this!
Edit: Everything here looks great! (Except #2237 😬 but that's an orthogonal problem) |
Fixes #2173.
While working on this I found this anomaly: #2237, please let me know if you think that one should be tracked down and addressed as part of this PR.