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

Added OnChange listener #16

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

Added OnChange listener #16

wants to merge 1 commit into from

Conversation

alexnix
Copy link

@alexnix alexnix commented Feb 12, 2016

I had the need to know to color the user selected in real time (while he is dragging the color selector) that is before onOk callback gets called so I implement an onChange callback that is called instantly when the user choses a color.

@denisk20
Copy link
Collaborator

Hi, thanks for your contribution! Do you think that real-time color update is a general scenario which should go into the core library? Sounds more like an edge use case which can be useful for a particular project, but which will just provide an extra empty method to implement for the majority of people.

@alexnix
Copy link
Author

alexnix commented Feb 12, 2016

You are right that could be some overhead code for most use cases. What do you say if I add another interface OnAmbilWarnaLiveListener that extends OnAmbilWarnaListener and this one has all three methods? Just in case anyone else is ever looking for this same functionality...

@denisk20
Copy link
Collaborator

It makes sense, but one thing that worries me is that onChange listener is called so many times during color selection, so it can easily become a performance bottleneck. For this feature's potential consumers I would recommend simply putting a comment into existing OnAmbilWarnaListener which would suggest that it is also possible to add onChange method to the interface and briefly describe a place in AmbilWarnaDialog where such method would be called (you could also put a comment inside AmbilWarnaDialog itself, something like //onChange would be called here. The comment should also warn a potential user about performance deterioration that the implementation of onChange may lead to.

@denisk20 denisk20 requested review from yukuku and removed request for yukuku July 5, 2017 09:05
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