- 
                Notifications
    
You must be signed in to change notification settings  - Fork 195
 
Various internal improvements #344
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
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.
Wow, what a new huge piece of work, thanks @Indigo744 ! All seems to be good to me (just a little question on a specific point).
I think I will have to take the time to dive deeply into the whole code in order to get all the parts in my mind since your last major (and great !) refactorings :)
| if (elem.options.text) elem.options.text.attrs.cursor = "pointer"; | ||
| } else { | ||
| // No HTML links, check if a cursor was defined to pointer | ||
| if (elem.mapElem.attrs.cursor === 'pointer') { | 
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.
Why did you add this check before setting the cursor style to 'auto' ?
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.
In case the user did set the cursor to something other than pointer (like help...)...
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.
In order to avoid overriding the user attribute.
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 thanks, I got it ! It will not work for the users who willingly set cursor to 'pointer', but it should be a very unusual case, and its still better than before :)
| 
           Storing the area / plot options within the objects next to mapElem and textElem is a great idea that allowed you to introduce a lot of simplifications , the elements configuration parts are a lot more clear now. I'm also glad to see the code factorization within the two new functions that you have introduced (setPlotAttributes and setPlotCoords).  | 
    
This PR implements some various internal improvements.
The main enhancement concerns the option: each element option is stored in the element object, alongside mapElem and textElem. This allows to simplify quite a bit of various other stuff related to the options, like HTML links options, tooltip options, event handlers...
Also, the getBBoxCenter is removed (fix #340).