Skip to content
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

add pan control-icons #250

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

add pan control-icons #250

wants to merge 1 commit into from

Conversation

mistyn8
Copy link

@mistyn8 mistyn8 commented Jun 24, 2017

Added pan control-icons for panning actions.. set at delta50.. might have to visit this to allow setting delta.

Added pan control-icons for panning actions.. set at delta50.. might have to visit this to allow setting delta.
@bumbu bumbu self-requested a review June 26, 2017 13:31
@bumbu
Copy link
Owner

bumbu commented Jun 26, 2017

Hi @mistyn8,

Thank you for your contribution. You did a very nice job!

But before merging a new functionality there are few things to consider:

  • It should not be breaking for current users - although it may seem that adding a new feature should never break any existing behaviour, it is less true with UI elements. There may be libraries that rely on zoom controls to be of a certain size and in a certain position. Or some implementations may have own custom pan controls. So adding new control elements for everyone may break the expected behaviour for some users.
  • It should be customisable - original zoom icons are not really customisable, and there were many requests and questions about that. Adding new elements that have even more things that can be customised - will bring even more complexity into the codebase. Some examples may be: customising icons positions, sizes, pan amount, disabling only pan controls, disabling only zoom controls...
  • It should be tested - ideally we should have covered anything that makes sense to test, and adding these controls as a core functionality will require that

An alternative option would be to add your work as an example of how to add custom controls into the demos section. This way those who need this functionality will have a good reference of how to use it, and we'll avoid all the potential issues and additional work that is required for adding this functionality into the core.

Let me know what you think about it. If you agree with the proposal you can update this PR, or create a new one and keep this one as a reference.

Best,
bumbu

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

Successfully merging this pull request may close these issues.

2 participants