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

App navigation refactor #12084

Merged
merged 16 commits into from May 23, 2024
Merged

Conversation

rtibbles
Copy link
Member

@rtibbles rtibbles commented Apr 15, 2024

Summary

  • Consolidates all app bar population to follow the same pattern as bottom app bar and side nav population
  • Updates the API for nav item registration to validate sub routes to allow populating the app bar
  • Consolidates the components for navigation bar display and overflow behaviour
  • Gets rid of navigation mixin in favour of limited composable
  • Folds navComponents logic into the composable
  • Gets rid of unused priority property
  • Cleans up all nav item registration to remove vestiges of when they were components rather than bare objects
  • Clean up Bottom App Bar navigation to ensure that something is always selected for the bottom app bar

References

Fixes #11731
Fixes #10395

Reviewer guidance

Test navigation in each of the SPAs, and also in app mode on a touch device to ensure that the bottom app bar is properly populating

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

🚀 This description was created by Ellipsis for commit 4762360

Summary:

Refactors app navigation by updating the registration API, introducing a useNav composable, and removing outdated patterns.

Key points:

  • Refactored app navigation to use a unified pattern across the app.
  • Updated API for navigation item registration to include sub-route validation.
  • Replaced navigation mixins with a new useNav composable for better state management.
  • Removed old navigation-related mixins and utilities.
  • Ensured navigation components are correctly populated and responsive to changes.

Generated with ❤️ by ellipsis.dev

@rtibbles rtibbles added the TODO: needs review Waiting for review label Apr 15, 2024
@github-actions github-actions bot added APP: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) APP: Facility Re: Facility App (user/class management, facility settings, csv import/export, etc.) APP: Learn Re: Learn App (content, quizzes, lessons, etc.) APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) APP: User Re: User app (sign-in, sign-up, user profile, etc.) DEV: frontend SIZE: very large labels Apr 15, 2024
@marcellamaki marcellamaki self-assigned this Apr 23, 2024
Copy link
Member

@marcellamaki marcellamaki left a comment

Choose a reason for hiding this comment

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

I think I may have more comprehension questions, but so as to get this moving again after delaying, I'll start with the questions I have, and then follow up as needed once I digest the answers :)

kolibri/core/assets/src/composables/useNav.js Outdated Show resolved Hide resolved
kolibri/core/assets/src/composables/useNav.js Outdated Show resolved Hide resolved
kolibri/core/assets/src/views/AppBar.vue Show resolved Hide resolved
kolibri/core/assets/src/views/AppBar.vue Outdated Show resolved Hide resolved
@pcenov
Copy link
Member

pcenov commented May 8, 2024

Hi @rtibbles, here are the issues I was able to identify:

  1. All the icons in the navigation bar are displayed in white instead of black:

icons

  1. The navigation bar disappears completely at the Profile and My downloads pages (this is especially unpleasant in the Mac app where the user can't navigate out of these pages):
profile-mydownloads.mp4
  1. The side bar disappears completely when I shrink the page to less than 840 pixels:

2024-05-08_16-30-32

The same is also valid for the Mac app:

2024-05-08_16-07-25.mp4
  1. When I change the language to any other language, the 'Sign out' button is not localized:

signout

@rtibbles
Copy link
Member Author

rtibbles commented May 8, 2024

These all seem like problems - although I thought I had handled the icon colour already.

One thing to note is that because I moved the location of the sign out string, this will remain unlocalized until we've done the translation pass to reinstate the translated string.

@rtibbles
Copy link
Member Author

Hey @ellipsis, give me a code review

Copy link

ellipsis-dev bot commented May 10, 2024

OK! Reviewing this PR...


Responding to this comment by @rtibbles. For more information about Ellipsis, check the documentation.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to 4762360 in 2 minutes and 43 seconds

More details
  • Looked at 2925 lines of code in 44 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. /kolibri/core/assets/src/views/AppBar.vue:19
  • Draft comment:
    The implementation of centralized navigation items using registerNavItem is well-executed. It simplifies the management of navigation items across different parts of the application. Good job on enhancing maintainability and consistency!
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The PR description and the changes made in the code indicate that the navigation items are now being registered and managed centrally, which is a significant improvement in terms of maintainability and consistency across the application. The use of registerNavItem in various components to define navigation items is consistent with the goal of centralizing navigation configuration. This approach not only reduces redundancy but also simplifies the management of navigation-related features.

Workflow ID: wflow_rqX52Vp7RqvnfxvQ


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

6 days left in your free trial, upgrade for $20/seat/month or contact us.

@rtibbles
Copy link
Member Author

@marcellamaki have addressed comments.

@pcenov I have fixed all the reported issues (except the internationalized Sign out, which will not be fixed until we redo translations, as noted).

@pcenov
Copy link
Member

pcenov commented May 17, 2024

Hi @rtibbles - I confirm that the reported issues are fixed now. While regression testing I spotted just one more - in mobile view the ellipsis button is positioned too close to the right, it's oval-shaped when clicked and the dots are white:

2024-05-17_17-05-10.mp4

Copy link
Member

@marcellamaki marcellamaki 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 good to go on my side, one the small final UI updated that Peter noticed is fixed. (I am also not opposed to that being follow up if it's not a super quick fix, but it seems like it might be easier to just fix rather than open an issue for it)

@rtibbles
Copy link
Member Author

Updated:
image

@rtibbles rtibbles merged commit 39bb9db into learningequality:develop May 23, 2024
31 checks passed
@rtibbles rtibbles deleted the horizontalnavbar branch May 23, 2024 16:12
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.) APP: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) APP: Facility Re: Facility App (user/class management, facility settings, csv import/export, etc.) APP: Learn Re: Learn App (content, quizzes, lessons, etc.) APP: User Re: User app (sign-in, sign-up, user profile, etc.) DEV: frontend SIZE: very large TODO: needs review Waiting for review
Projects
None yet
3 participants