-
Notifications
You must be signed in to change notification settings - Fork 50
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
Colorful + animated progress bars #215
Colorful + animated progress bars #215
Conversation
I like this! Hope to see it in the next release. |
I have yet to test this fully but I noticed one thing right away: you can't just remove a config option, you have to add logic to migrate it from old style enum to your checkboxes to not lose user preferences. There are some examples of this in config class, see And default should be the same: animated, not colorful. |
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.
Ok, this looks good, just one more thing to address:
There is a lot of common logic of determining the progress bar variant in torrenttable.tsx
and details.tsx
, extract that into a single util function, you can put it in utils.js
.
@qu1ck For the deprecated setting in the
|
Yes, that's fine. |
Friendly ping. |
Sure, was on vacation and back now. Halfway done with suggestions |
@qu1ck Check out latest changes |
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.
Fix the eslint issues please
248a1cb
to
f2a28c0
Compare
Done @qu1ck |
f2a28c0
to
1cd3d0c
Compare
Looks good, just one question. Was it intentional that when animations are disabled then torrents will be only blue/red/green? Why disable other colors? |
I was conservative and just kept the original state of things for the original options, while adding a new combination option. Could expand the new colors to the "colorful" option if we're feeling frisky |
Well in previous implementation things were either colorful or animated, they couldn't be both. But since this change has 2 different settings for both it makes sense that the difference between [colors on, animations off] and [colors on, animations on] would be only animations, the colors would stay the same. |
@melyux can you at least comment if you are going to do change discussed above? |
Since you approved it as is already I thought it was for a future release. I can make the changes now then |
Please do. |
@qu1ck Done with the changes, but since you force pushed the branch (somehow!), it won't let me push again. How can I fix it? |
I just rebased your branch without changes. You can force push again. |
1cd3d0c
to
dedece5
Compare
@qu1ck Done |
dedece5
to
e6a29c9
Compare
Thanks for your contribution and patience. You still have one more PR open :) |
Re-implementation of the PR #82 from last year, on top of the recent change 05303b6. Adds the third possibility of using colorful + animated together (color based on status, animated based on activity):
Takes into account all of @qu1ck's suggestions from the original PR. Fits things to match the existing color scheme and the Transmission app:
Only applies if user has both "colorful" and "animated" selected for progress bars in interface settings. Other combinations are unaffected by this.