Colorful progress bars #82
Conversation
|
This is exciting! Thanks! |
|
|
||
| interface ProgressBarProps { | ||
| now: number, | ||
| max?: number, | ||
| label?: string, | ||
| animate?: boolean, | ||
| status?: number, |
There was a problem hiding this comment.
Status logic should not be in ProgressBar component because it is generic. It should have an enum variant prop that gives it correct color, determine the variant in parent component that knows what status means, let progressbar handle the color based on variant.
There was a problem hiding this comment.
Kind of confused on this one. Do you mean the ProgressBar component should have an enum for the possible colors, and everything that calls ProgressBar should manually convert e.g. the torrent status into a ProgressBarColor? Which would mean repeating the code between torrenttable.tsx and details.tsx?
There was a problem hiding this comment.
No need to repeat the code, extract it into a helper function, you can put it in torrent.ts.
| const label = props.label ?? `${percent}%`; | ||
| const className = `progressbar ${props.animate === true ? "animate" : ""} ${props.className ?? ""}`; | ||
| const label = props.label || `${percent}%`; | ||
| const animate = (props.animate && props.status !== Status.queuedToVerify && |
There was a problem hiding this comment.
Animate prop should also be taken as is. Determine whether progress bar should be animated in the parent.
| props.status == Status.magnetizing || props.status == Status.verifying; | ||
| let color = "blue"; | ||
|
|
||
| const config = useContext(ConfigContext); |
There was a problem hiding this comment.
Move the config checks outside as well.
Basically this should be a dumb generic reusable component that just draws what it's told.
| @@ -136,7 +136,7 @@ function ByteSizeField(props: TableFieldProps) { | |||
| function PercentBarField(props: TableFieldProps) { | |||
| const now = props.entry.percent ?? 0; | |||
|
|
|||
| return <ProgressBar now={now} className="white-outline" />; | |||
| return <ProgressBar now={now} status={Status.downloading} className="white-outline" />; | |||
There was a problem hiding this comment.
Keep this blue.
Or, if you want to get colorful here too then make skipped files grey, downloading ones blue and finished ones green to be consistent with torrent table.
| @@ -80,7 +81,8 @@ function PercentField(props: TableFieldProps) { | |||
| return <ProgressBar | |||
| now={now} | |||
| className="white-outline" | |||
| animate={active} />; | |||
| animate={active} | |||
| status={Status.downloading} />; | |||
There was a problem hiding this comment.
Same as above, either keep this unchanged or keep consistent logic.
| status = Status.error; | ||
| } else if (props.torrent.status === Status.downloading && props.torrent.pieceCount === 0) { | ||
| now = 100; | ||
| label = "🧲"; |
There was a problem hiding this comment.
No emojis and no custom labels here, the icon in name field is enough indication.
| if ((props.torrent.error !== undefined && props.torrent.error > 0) || props.torrent.cachedError !== "") { | ||
| status = Status.error; | ||
| } else if (props.torrent.status === Status.downloading && props.torrent.pieceCount === 0) { | ||
| now = 100; |
There was a problem hiding this comment.
Not sure if this is leftover from testing or you intend to always show full progress bar. I'm inclined to not show it, even if it makes the color of the progress bar rarely visible because it usually goes from 0 to downloading in a single update.
There was a problem hiding this comment.
The problem with the Magnetizing one is that I think you said the Magnetization progress is not grabbed at the full table level, only at the per-torrent level. So the Magnetization process will never be shown, and so the colors will never be shown. I felt it's better to have the bar be full all the time to at least show that this torrent is being Magnetized.
There was a problem hiding this comment.
Makes sense, showing full animated bar with no percentage text will be fine.
| .progressbar.dark-blue>:first-child { | ||
| background: #0054ae; | ||
| } | ||
|
|
||
| .progressbar.green>:first-child { | ||
| background: #36B24D; | ||
| } | ||
|
|
||
| .progressbar.dark-green>:first-child { | ||
| background: #1d8931; | ||
| } | ||
|
|
||
| .progressbar.red>:first-child { | ||
| background: #FA5352; | ||
| } | ||
|
|
||
| .progressbar.dark-red>:first-child { | ||
| background: #C92B2A; | ||
| } |
There was a problem hiding this comment.
The colors are fine but they are not from the default palette and stand out.
This and the default blue should be moved from css file into inline css in jsx and use the MantineTheme colors.
If you are not sure how to do that I can move current styles there and you can build on top of it.
| magnetizing: -1, | ||
| error: -2, |
There was a problem hiding this comment.
This file should not be changed at all. It is meant to directly map to transmission API.
Once you move the status out of ProgressBar you will likely find that you don't need these fake statuses here.
|
Any update on this? |
|
Will work on the PR suggestions this week |
|
@melyux any progress on this? We recently got an issue that could be solved by your PR where users may benefit from having your color-changing progress bars instead of animated progressbars that take up CPU/GPU on low-spec devices so I think your work has inadvertently solved another issue that certain users have. #170 |
|
This is now implemented in a similar way in 05303b6 |
First time writing in this lang. Part of #40
CC: @Pentaphon