-
Notifications
You must be signed in to change notification settings - Fork 6
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
22059 - loading animation for loading business info. #46
Conversation
/gcbrun |
Temporary Url for review: https://business-dashboard-dev--pr-46-n5t92760.web.app |
/gcbrun |
Temporary Url for review: https://business-dashboard-dev--pr-46-n5t92760.web.app |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey nice work, just the one comment if it makes sense to you too
src/app.vue
Outdated
class="animate-spin text-[50px] text-gray-700 absolute top-40 left-[50%]" | ||
/> | ||
<div v-if="dashboardIsLoading || appLoading"> | ||
<BcrosLoadingIcon class="animate-spin text-6xl text-gray-700 absolute top-40 left-[50%]" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we could move this to the layout? That way it could include covering the business details header. I think it should work pretty good if we replace the 'appLoading' with your new 'trackUiLoadingStart/Stop' inside the onMounted in here. That way the business details would always get the cached business info too so it would only make the one call
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved them to the layouts, and replaced main loading with track loading stuff.
src/stores/ui.ts
Outdated
const trackUiLoadingStop = (trackerUuidDone: string) => { | ||
const index = uiIsLoading.value.find(tracker => tracker === trackerUuidDone) | ||
if (index !== -1) { | ||
uiIsLoading.value.splice(index, 1) | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should use findIndex
since find
will return the value not the index.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, this is great spot
cypress/e2e/initial-page-load.cy.ts
Outdated
cy.visit(`/${businessIdentifier}`) | ||
|
||
cy.get('[data-cy="loading-icon"]').should('be.visible').then(() => { | ||
cy.wait(['@getBusinessInfo']) | ||
cy.get('[data-cy="loading-icon"]').should('not.exist') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: why do we just wait @getBusinessInfo
? Do we need to wait for the other calls to be intercepted (like getTasks) before the loading icon disappears?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, mandatory one is to wait for 2nd load business call. Generally we could wait for others too, but the thing is there is ~5 sec wait anyways if it is not immediately available. And if it fails with timeout we should investigate why. So its not necessary here.
/gcbrun |
Temporary Url for review: https://business-dashboard-dev--pr-46-n5t92760.web.app |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work.
*Issue:22059 *bcgov/entity#22059
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the namex license (Apache 2.0).