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

LWS-209: Add spinner #1080

Merged
merged 1 commit into from
Jul 30, 2024
Merged

LWS-209: Add spinner #1080

merged 1 commit into from
Jul 30, 2024

Conversation

jesperengstrom
Copy link
Contributor

@jesperengstrom jesperengstrom commented Jul 5, 2024

Description

Adds a loading indicator

Tickets involved

LWS-209

Solves

No indication of anything happening when for example searching.

Summary of changes

  • Added a progress bar (rather than a spinner) using the nProgress package. It runs on every navigation but i've added a small delay to avoid showing it on instant navigations. This can of course be tweaked further.
  • With this in place, I tried removing the streaming of search results on resource pages. This would otherwise require a second spinner to replace the 'loading' text. I think this works well - with the indicator in place, it's quite satisfying to get the whole page at once.
  • Made some minor related adjustments to the load function on the resource page. The getRelated() / await getRelated() call was done as the last thing before returning, not optimal when everything else is ready. We want to do it as soon as possible (i.e when there's a resourceId), but not wait for it, blocking subsequent operations. So we now make the call early and store the promise in a variable, awaiting it at the very end.

@jesperengstrom jesperengstrom marked this pull request as ready for review July 5, 2024 10:12
Copy link
Contributor

@johanbissemattsson johanbissemattsson left a comment

Choose a reason for hiding this comment

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

LGTM! Awesome –  I agree that it's feels much more responsive and satisfying to navigate around now! 🌟

Good call with the getRelated promise 👍 We could maybe also achieve the same effect by implementing multiple parallel promises using promise.all in (or near) the return (instead of having to declare them as variables beforehand) but I think it's best done if we can do it together while pair programming.

Copy link
Contributor

@olovy olovy left a comment

Choose a reason for hiding this comment

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

LGTM
Agree that it feels more responsive!

Merging this.

@olovy olovy merged commit 0d43d02 into develop Jul 30, 2024
1 check passed
@olovy olovy deleted the feature/LWS-209-spinner branch July 30, 2024 15:08
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