Skip to content

Conversation

@VilppeRiskidev
Copy link
Collaborator

@VilppeRiskidev VilppeRiskidev commented Mar 25, 2025

  • Added a corresponding vuex mutation
  • Separated indexOfDownloadProgress function
  • Removed the "There was a mismatch between download update's assign ID" check as that is now expected behavciour when a download is removed from the list.

@VilppeRiskidev VilppeRiskidev self-assigned this Mar 25, 2025
@VilppeRiskidev
Copy link
Collaborator Author

VilppeRiskidev commented Mar 25, 2025

"There was a mismatch between download update's assign ID" behaviour is now normal, unless there's better way of implementing this, so I removed the logging for that.

@VilppeRiskidev VilppeRiskidev removed the request for review from anttimaki March 25, 2025 11:53
@VilppeRiskidev VilppeRiskidev force-pushed the clear-inactive-downloads branch from 1f48b8e to 6d28ade Compare March 25, 2025 12:02
Copy link
Collaborator

@anttimaki anttimaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, code-wise this looks good to me. Couldn't resist leaving some nitpicks related to code style, but they're not blockers.

I would perhaps consider the UI again though. The remove button is positioned kind of awkwardly. Vertically it doesn't seem to be aligned in any sensible way (I know it's positioned at the top of its container but that's not what the user sees due to other whitespace around it). Horizontally there's less margin between the button and progress bar than the button has on its other sides.

Take into account that in the future the failed downloads would have two buttons here, "remove" and "retry". So something smaller like icon-sized button, without the borders and padding of the button, might be easier to deal with. I can't recall if there's support for such buttons in the code base currently.

And to clarify the UI side isn't necessarily a blocker either.

@VilppeRiskidev VilppeRiskidev force-pushed the clear-inactive-downloads branch from 6d28ade to 62e731f Compare March 26, 2025 10:17
@anttimaki
Copy link
Collaborator

@VilppeRiskidev What's the status of this PR? There's a force push but doesn't seem the review comments are addressed one way or another.

…Monitor

- Added a corresponding vuex mutation
- Separated indexOfDownloadProgress function
- Removed the "There was a mismatch between download update's assign ID" check as that is now expected behavciour when a download is removed from the list.
@VilppeRiskidev VilppeRiskidev force-pushed the clear-inactive-downloads branch from 62e731f to 3eecfce Compare April 1, 2025 18:10
@VilppeRiskidev
Copy link
Collaborator Author

The comments have been addressed.
Action button's styles look good to my eye now 🤷‍♂️ .

@VilppeRiskidev VilppeRiskidev requested a review from anttimaki April 1, 2025 18:12
Copy link
Collaborator

@anttimaki anttimaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The icon is a bit large but otherwise this looks nice now.

@anttimaki anttimaki merged commit 06c67a2 into develop Apr 2, 2025
5 checks passed
@anttimaki anttimaki deleted the clear-inactive-downloads branch April 2, 2025 06:27
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.

3 participants