Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix global loading state on root Coach pages #11218

Merged
merged 2 commits into from
Sep 11, 2023

Conversation

MisRob
Copy link
Member

@MisRob MisRob commented Sep 11, 2023

Summary

  • Removes global loader from the Coach Classes page since there is already the local one
  • Fixes missing global loader on root Coach pages and tabs

See commit messages for more details.

Before After
coach-before coach-after

Comments

Further improvements in the Coach loading states and navigation overall are needed. See #11219.

References

Closes #10729

Reviewer guidance

On slow network, navigate the root Coach pages and tabs

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

Fixes both global and local loaders
being displayed when loading the
classes page.
on root coach pages.

Even though global/local loading state is set in some
loading pages handlers, before running individual pages
handlers, coach plugin fetches some data in global 'beforeRoute'.
That takes significantly more time than individual handlers.
Not having loading state set in this 'beforeRoute' caused
seemingly missing global loader (even though it was displayed very
briefly for individual handlers)
@github-actions github-actions bot added APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) SIZE: very small labels Sep 11, 2023
@MisRob MisRob added the TODO: needs review Waiting for review label Sep 11, 2023
@MisRob MisRob marked this pull request as ready for review September 11, 2023 12:01
@MisRob MisRob requested a review from pcenov September 11, 2023 12:10
Copy link
Member

@akolson akolson left a comment

Choose a reason for hiding this comment

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

Changes look correct to me! Thanks @MisRob!

@@ -26,7 +26,8 @@ export default [
path: '/classes',
component: CoachClassListPage,
handler(toRoute) {
store.dispatch('loading');
// loading state is handled locally
store.dispatch('notLoading');
Copy link
Member

Choose a reason for hiding this comment

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

This accords with my general sense that we can reduce the role of the core loading state to be about toggling the linear loader when the navigation is still resolving. In this frame, we could handle it entirely within our core routing setup, where we toggle it on before route enter, and toggle it after after the route has resolved.

@rtibbles rtibbles merged commit 1acff36 into learningequality:release-v0.16.x Sep 11, 2023
@MisRob MisRob deleted the coach-loading-state branch February 29, 2024 08:41
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) SIZE: very small TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update loading state in Coach
3 participants