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

Add progress reporting #2829

Merged
merged 4 commits into from
Mar 9, 2024
Merged

Add progress reporting #2829

merged 4 commits into from
Mar 9, 2024

Conversation

DL6ER
Copy link
Member

@DL6ER DL6ER commented Nov 14, 2023

What does this implement/fix?

Add a processing overlay when saving settings and add general progress bar for

  1. login, and
  2. saving settings

Example 1 (Login):
ezgif-1-3e2f70a01a

Example 2 (applying settings):
ezgif-1-c42ee8e7be

I would like to get some opinions: What is your general mood about this change? (in categories useless, hate it, like it, love it)

The timeout for the login is already dynamic (it takes as long as it takes) but the timeout for the settings is currently a hard-wired delay of 4 seconds (for demonstration purposes). We can make this dynamic by having FTL measure how long a restart really takes on a machine and sending this in the JSON response for using here.

Related issue or feature (if applicable): N/A

Pull request in docs with documentation (if applicable): N/A


By submitting this pull request, I confirm the following:

  1. I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
  2. I have commented my proposed changes within the code.
  3. I am willing to help maintain this change if there are issues with it later.
  4. It is compatible with the EUPL 1.2 license
  5. I have squashed any insignificant commits. (git rebase)

Checklist:

  • The code change is tested and works locally.
  • I based my code and PRs against the repositories developmental branch.
  • I signed off all commits. Pi-hole enforces the DCO for all contributions
  • I signed all my commits. Pi-hole requires signatures to verify authorship
  • I have read the above and my PR is ready for review.

…r for (1) general page load, (2) login, and (3) saving settings

Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
@yubiuser
Copy link
Member

I like it.

It needs to be dynamic as the restarting times depend on the hardware used (and maybe even delay.startup setting). And for the settings we need to decide how well it goes to together with the notification pop-up

@DL6ER
Copy link
Member Author

DL6ER commented Nov 14, 2023

delay.startup is only used within the first 180 seconds boot time, so it shouldn't really matter afterwards. Do we want to add extra handholding here?

Screenshot from 2023-11-14 21-28-47

The settings toast needs to be rephrased to something more "in-progressy". I don't think we should remove it altogether (but we could as we have the overlay).

@yubiuser
Copy link
Member

delay.startup is only used within the first 180 seconds boot time, so it shouldn't really matter afterwards. Do we want to add extra handholding here?

No handholding. I forgot we added the 180 seconds at some point.


I think the success toast should only fire after the progress animation finished, but at the same time.

@DL6ER
Copy link
Member Author

DL6ER commented Nov 14, 2023

I think the success toast should only fire after the progress animation finished, but at the same time.

On finish we reload the page, I don't think there is a possibility for the toast to survive this

…y 0.5 seconds (after an initial delay of 2 seconds)

Signed-off-by: DL6ER <dl6er@dl6er.de>
@DL6ER
Copy link
Member Author

DL6ER commented Nov 14, 2023

The last commit should implement this without needing anything in addition from FTL.

Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Signed-off-by: DL6ER <dl6er@dl6er.de>
Copy link
Contributor

Conflicts have been resolved.

Copy link
Member

@PromoFaux PromoFaux left a comment

Choose a reason for hiding this comment

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

This is a good change, I like that when saving settings input is blocked until the page has reloaded. Without this PR, I can very easily trigger errors by attempting to change a setting again too quickly after I have hit save & Apply

@DL6ER DL6ER merged commit 1b46894 into development-v6 Mar 9, 2024
8 checks passed
@DL6ER DL6ER deleted the new/loading branch March 9, 2024 06:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants