czue / celery-progress

Drop in, configurable, dependency-free progress bars for your Django/Celery applications.
MIT License
473 stars 90 forks source link

Custom Colors #62

Closed EJH2 closed 4 years ago

EJH2 commented 4 years ago

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:

CeleryProgressBar.initProgressBar(taskURL, {barColors: {'progress': '#ef4'}})

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.

EJH2 commented 4 years ago

I don't think the default value documentation is the best looking, so I'm definitely open to suggestions on that.

EJH2 commented 4 years ago

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.

OmarWKH commented 4 years ago

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.

EJH2 commented 4 years ago

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.

OmarWKH commented 4 years ago

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.

@czue @EJH2

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 :/

EJH2 commented 4 years ago

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 CeleryProgressBar instead of this, though.

OmarWKH commented 4 years ago

You are absolutely right.