Skip to content

Conversation

@susnux
Copy link
Contributor

@susnux susnux commented Feb 26, 2025

This feature implements the following:

  • Implement the ETA class our self
  • Provide upload speed information in the ETA class
  • Move the ETA information to the uploader class
  • Remove custom ETA from uploader component and add uploading speed.

Due to the risks of something breaking this is 100% test covered (unit tests) and additionally added component tests for it.

@susnux susnux added enhancement New feature or request 3. to review Waiting for reviews labels Feb 26, 2025
@susnux susnux marked this pull request as ready for review February 26, 2025 21:58
@susnux susnux requested review from artonge and skjnldsv February 26, 2025 21:58
Copy link
Contributor

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

Super good! Tested and works great!
We could need slight changes though:

  1. The speed can be cut of when estimating
    (probably best to hide it when estimating ?)
    image
  2. When very high speed, I barely got time to see the speed or time left, it jumps from "estimating" to "assembling", do you think we can do better here? 🤔
  3. When assembling, we should probably hide the cancel button ?

@susnux
Copy link
Contributor Author

susnux commented Feb 27, 2025

Super good! Tested and works great! We could need slight changes though:

Thank you!

1. The speed can be cut of when estimating (probably best to hide it when estimating ?)

Makes sense, I will think of a good way :)

2. When very high speed, I barely got time to see the speed or time left, it jumps from "estimating" to "assembling", do you think we can do better here? 🤔

We can check the current speed if it will be just a few seconds and directly show "few seconds left" instead of estimating?

3. When assembling, we should probably hide the cancel button ?

Makes sense!

@skjnldsv
Copy link
Contributor

We can check the current speed if it will be just a few seconds and directly show "few seconds left" instead of estimating?

Good option! 👍

susnux added 6 commits March 3, 2025 09:37
This feature implements the following:
- Implement the ETA class our selfe
- Provide upload speed information in the ETA class
- Move the ETA information to the uploader class
- Remove custom ETA from uploader component and add uploading speed.

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
"a few seconds left" is so long that there is no space.
So we only show it when there is enough space - otherwise just as a
title attribute.

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux
Copy link
Contributor Author

susnux commented Mar 3, 2025

All points resolved I think :)

@susnux susnux requested a review from skjnldsv March 3, 2025 09:33
@susnux susnux mentioned this pull request Mar 4, 2025
@skjnldsv skjnldsv merged commit 00bc4cb into main Mar 4, 2025
22 checks passed
@skjnldsv skjnldsv deleted the feat/eta branch March 4, 2025 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Show current upload speed

3 participants