Skip to content
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

PB-726: Added a print progress bar instead of a spinner - #patch #967

Merged
merged 2 commits into from
Jun 27, 2024

Conversation

ltshb
Copy link
Contributor

@ltshb ltshb commented Jun 26, 2024

The progress bar is degressive and it's completion time is set based on the
number of layer to print

Test link

@github-actions github-actions bot changed the title Added a print progress bar instead of a spinner Added a print progress bar instead of a spinner - #minor Jun 26, 2024
Copy link

cypress bot commented Jun 26, 2024

Passing run #2799 ↗︎

0 211 21 0 Flakiness 0

Details:

Fix flaky test
Project: web-mapviewer Commit: c2b35921fe
Status: Passed Duration: 04:48 💡
Started: Jun 27, 2024 2:21 PM Ended: Jun 27, 2024 2:26 PM

Review all test suite changes for PR #967 ↗︎

Copy link
Member

@hansmannj hansmannj left a comment

Choose a reason for hiding this comment

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

Nice idea! 🥇

Might also make sense, once the printing itself is through and the progess bar is at 100% and disappears, to have a small message there?
When testing on the train with a slow connection to print an A3, the progress bar nicely shows that printing is done.
But downloading an A3 takes quite long and there is no more progress bar of course for that.
But a small message like "downloading PDF" or something might make the user feel more comfortable?

@ltshb ltshb changed the title Added a print progress bar instead of a spinner - #minor PB-726: Added a print progress bar instead of a spinner - #minor Jun 27, 2024
@ltshb
Copy link
Contributor Author

ltshb commented Jun 27, 2024

@hansmann for the download time is difficult and behave differently on mobile device and desktop device. On desktop the download is left to the browser and you have the browser download file progress, while on mobile the file is open in a new window.
An issue fix could be to slightly change the success message from "Operation Successful!" to "Print Successful! Downloading..." but then even if the download is done the message will stay so not sure it helps.

@pakb
Copy link
Contributor

pakb commented Jun 27, 2024

I don't really like that we do a magic number calculation to decide how to progress the bar, that might backfire in case of future outages, or unforeseen delay (bar being stuck at 99%, like in everyday life on Windows...)

There's no way of giving some steps/progress/feedback from the polling request of the backend?

@ltshb
Copy link
Contributor Author

ltshb commented Jun 27, 2024

I don't really like that we do a magic number calculation to decide how to progress the bar, that might backfire in case of future outages, or unforeseen delay (bar being stuck at 99%, like in everyday life on Windows...)

There's no way of giving some steps/progress/feedback from the polling request of the backend?

@pakb Unfortunately not that I'm aware off. However your issue is mitigated with https://github.com/geoadmin/infra-kubernetes/pull/568 were the backend timeout after 3min and with my logic here having a degressive progress bar, it will never stays long at 99%, in the worst case for a single layer print it might stay stuck ~1min. We could either decrease the backend timeout to 2min in my opinion ?

@hansmannj
Copy link
Member

@hansmann for the download time is difficult and behave differently on mobile device and desktop device. On desktop the download is left to the browser and you have the browser download file progress, while on mobile the file is open in a new window. An issue fix could be to slightly change the success message from "Operation Successful!" to "Print Successful! Downloading..." but then even if the download is done the message will stay so not sure it helps.

Exactly, I don't mean to have a progress bar for the download, but a message, because the download time is unpredictable

@hansmannj
Copy link
Member

I don't really like that we do a magic number calculation to decide how to progress the bar, that might backfire in case of future outages, or unforeseen delay (bar being stuck at 99%, like in everyday life on Windows...)
There's no way of giving some steps/progress/feedback from the polling request of the backend?

@pakb Unfortunately not that I'm aware off. However your issue is mitigated with geoadmin/infra-kubernetes#568 were the backend timeout after 3min and with my logic here having a degressive progress bar, it will never stays long at 99%, in the worst case for a single layer print it might stay stuck ~1min. We could either decrease the backend timeout to 2min in my opinion ?

Timeout of 2 min might do, just need to test with A3 and a ton of layers. 2 Minutes might be fully used for those cases...

Copy link
Contributor

@boecklic boecklic left a comment

Choose a reason for hiding this comment

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

imho in general a good idea to show to users that print takes time. It's not ideal to set if fixed - arbitrary - value for when it's done, but we don't have a better option right now.

@ltshb
Copy link
Contributor Author

ltshb commented Jun 27, 2024

@pakb do you agree to use this ?

The progress bar is degressive and it's completion time is set based on the
number of layer to print
@pakb
Copy link
Contributor

pakb commented Jun 27, 2024

@pakb do you agree to use this ?

I don't disagree, I just feel it's a bad solution to a bad problem 😉 (you have a consensus, I think 🤔 )

EDIT: make it #patch maybe 😉

@ltshb ltshb changed the title PB-726: Added a print progress bar instead of a spinner - #minor PB-726: Added a print progress bar instead of a spinner - #patch Jun 27, 2024
@ltshb ltshb merged commit e56b5c0 into master Jun 27, 2024
6 checks passed
@ltshb ltshb deleted the feat-print-progress branch June 27, 2024 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants