-
Notifications
You must be signed in to change notification settings - Fork 91
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
Custom Colors #62
Custom Colors #62
Conversation
I don't think the default value documentation is the best looking, so I'm definitely open to suggestions on that. |
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.
@EJH2 on a roll! 👏 👏
Does the static method change have any unexpected side effects? e.g. if someone was referencing those functions directly in their code for some other purpose? (not exactly sure when/why that would happen, just want to understand the implications...)
I don't see any immediate issues with making these methods non-static from my testing, but @OmarWKH might be better equipped to comment on potential side-effects of this change. |
I think if someone was referencing one of the changed static methods this PR will break their code. But I don't think users are referencing them and the documentation doesn't direct them to do so. |
To be fair, removing stop_task is a breaking change and we didn't really give much thought to that. So I'll go ahead and merge this in so I can start on the next one. |
While investigating #66. I realized there's an obvious use case to reference the default handlers. You may want to do something upon success but then call the default success handler to show the result. Might be too late :/ |
Wouldn't this change make it theoretically easier to accomplish that, since you can just do onSuccess: function(progressBarElement, progressBarMessageElement, result) {
console.log(progressBarElement, progressBarMessageElement, result);
this.onSuccessDefault(progressBarElement, progressBarMessageElement, result);
} in the bar declaration? Now that you mention it, this PR does the old reference method of having to type |
You are absolutely right. |
Resolves #5
This PR aims to add the ability to change progress bar colors in a (noob-)friendly fashion (basically not having to override the whole function just to change one line). Usage would look as follows:
The above example would change the progress bar color to yellow. Any colors that aren't expressly overridden will defer to the default value, as specified in the documentation. This would also allow us to add new colors (such as IGNORED) without necessarily needing to break everything if people are only overriding for color changes.