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

Refactored HomePage route handler to fetch initClassInfo and getFacil… #12358

Merged
merged 20 commits into from
Jul 12, 2024

Conversation

shubh1007
Copy link
Contributor

Summary

This is the continuation of the refactoring process for the coach page, transitioning from a global route handler to local route handlers. #11219

References

List of all planRoutes:

Route name Handler/component to refactor Refactor done? How to test the page
PageNames.HOME_PAGE showHomePage yes Coach -> Class Home
HomeActivityPage.name showHomePage yes Coach -> Class Home -> activity

Reviewer guidance

Please follow the table for reviewing the refactored routes.

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
  • Documentation is updated
  • Contributor is in AUTHORS.md

@github-actions github-actions bot added APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) DEV: frontend labels Jun 25, 2024
@MisRob
Copy link
Member

MisRob commented Jun 25, 2024

Hi @shubh1007, this is great! I can see you grasped the problem and the strategy for solving it, thanks a lot. This will really help us to unblock some important work. Leaving just a few details, but overall looks really good.

Do you wish to continue with some other routes from this file? If so, would you push them to this PR as well or would you rather work in a new one?

Note that I'm mostly offline until July 8 - just chiming in on this issue to confirm that you can continue if you'd like, and on some days I will be available to answer questions.

kolibri/plugins/coach/assets/src/app.js Outdated Show resolved Hide resolved
kolibri/plugins/coach/assets/src/routes/index.js Outdated Show resolved Hide resolved
kolibri/plugins/coach/assets/src/routes/index.js Outdated Show resolved Hide resolved
@MisRob
Copy link
Member

MisRob commented Jul 4, 2024

Thanks for following up @shubh1007. I will finish review when I'm back online next week. Meanwhile, I feel confident that we're on the same page about this task so feel free to continue working on other routes if you'd like (or please message us if you don't want to work on more so we know we should re-assign)

and remove hardcoded colors
in favor of theme tokens.

Fixes learningequality#12425
This fixes vue/require-v-for-key 'Elements in iteration expect to have 'v-bind:key' directives.'
lint error and ensures efficient Vue updates.
@MisRob MisRob self-assigned this Jul 9, 2024
Copy link
Member

@MisRob MisRob left a comment

Choose a reason for hiding this comment

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

@shubh1007 Thank you. I pushed two last tweaks. See commit messages to understand. As soon as checks pass, we will merge.

@MisRob MisRob merged commit 402f18e into learningequality:develop Jul 12, 2024
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) DEV: frontend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants