Skip to content

Conversation

ssaki
Copy link

@ssaki ssaki commented Dec 30, 2014

Fixes #10737

~ Extracted the code responsible for translation of values into slider handle's distances into _calculateDistance() (was in _calculateDistance())
~ Extracted the code responsible for translation of slider handle's distance into numeric values into _calculateValue() (was in _normValueFromMouse())
~ Added two new callback options and modified the constructor to set the new methods as default values (if not already set)
~ cleaned up all variables that became unused due to the re-factoring
~ updated the common test case to reflect the change in options

Alexander Marinov added 2 commits December 30, 2014 13:44
Fixes #10737 

~ Extracted the code responsible for translation of values into slider handle's distances into _calculateDistance() (was in _calculateDistance())
~ Extracted the code responsible for translation of slider handle's distance into numeric values into _calculateValue() (was in _normValueFromMouse())
~ Added two new callback options and modified the constructor to set the new methods as default values (if not already set)
~ cleaned up all variables that became unused due to the re-factoring
~ updated the common test case to reflect the change in options
Fixes #10737 

fixed trailing space issues to make Travis happy
@tjvantoll
Copy link
Member

Hey @ssaki,

I think this is a reasonable use case for an extension point, but we don't want to introduce any new options. Can you achieve the behavior you're looking for without the new options?

@arschmitz
Copy link
Member

@ssaki @tjvantoll The slider is due to be re-written for both UI and Mobile very soon so perhaps this is something to keep in mind for that rather then add right now.

@ssaki
Copy link
Author

ssaki commented Dec 31, 2014

@tjvantoll, yes, it would be basically the same code just without the options and using direct method calls instead of callbacks via the options. That way one would have to extend the plugin, but this is a trivial operation and there is descent documentation on how to do it. Perhaps an example in the slider section will be even better

@arschmitz I personally don't mind, but please have it in mind when time comes :) IMO the extension point is necessary to reduce maintenance efforts in case of customization as currently the cleanest way is to extend the plugin and copy/modify both involved methods, which have lot of unrelated (to the calculation itself) logic.

@arschmitz
Copy link
Member

@ssaki I agree completely with having an extension point. I was only saying we may want to wait for the new widget to add it. We generally don't add anything new to widgets that are going to be re-written, until the re-write.

@jzaefferer
Copy link
Member

I think its fine to work this out before the rewrite. This PR needs an update, as TJ described above. It also missing unit tests that cover this extension. And this should be accompanied by a API docs PR to document the new extension point(s), similar to, for example, _isDivider in menu.

@scottgonzalez
Copy link
Member

Since this involves public API, please make proposals for the new APIs in the ticket. Once we have consensus on the API, then you can update the PR.

@scottgonzalez
Copy link
Member

Closing due to inactivity. If you're still interested in working on this, please start by proposing the actual API in the ticket.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants