Skip to content

Conversation

okwme
Copy link
Contributor

@okwme okwme commented Mar 8, 2016

No description provided.

@neveldo
Copy link
Owner

neveldo commented Mar 9, 2016

Hello @billyrennekamp ,

Thank you for your PR !! However, you hould have kept the changes into separated pull-requests, as its easier for us to review and discuss on them. Maybe you could make again the PR separately ?

Regarding the fix commit "error with undefined var plots when afterUpdate is being called", it's ok for me.

Regarding the new minLevel option, I see the goal of it. However, I think it could lead to strange behaviour if a user set this option without setting the same value into options.map.zoom.init.level. Could you provide a JSFiddle that show this new option in use in order to test it ? Maybe, in the init() function, we should do self.options.map.zoom.init = self.options.map.zoom.minLevel if minLevel is set.

Regarding the fix for hover behaviour between text and area, great fix, it's ok for me ! I have just a little comment to make about the variable name hoverFROM. Actually, the TO from hoverTO was for "TimeOut". Maybe hoverTO could be renamed to mouseoverTimeout, and hoverFROM to mouseoutTimeout, what do you think ?

I'm not agree for the dx, dy feature as it lead to duplicated code. See this comments : #186 (comment) , I will make a PR for this ASAP.

@neveldo
Copy link
Owner

neveldo commented Mar 9, 2016

Here we go for the improved margin feature : #204

Could you check it on your project and give me a feedback ?

However, FYI, I have just found a little bug with text position that need to be fixed : #203

@okwme okwme closed this Mar 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants