-
Notifications
You must be signed in to change notification settings - Fork 29
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
New: Add option to show course-level progress on PLP nav button (fix #226) #227
Conversation
I'm wondering (for the sake of a clean & streamlined AAT UI) if we could combine the following two configuration options:
Once combined, it would be a dropdown choice of |
Please don't do this yet. Wait for migration scripts. |
_showAtCourseLevelInNavigationBar to _useCourseProgressInNavigationButton, to match the description? |
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.
Functionality wise works exactly as expected. Just one minor change, currently the aria label for the nav button still says page progress when it's showing course progress, think it should say course progress?
Also feels a bit odd that it could potentially show course progress in the button but only that page's items in the drawer, not sure if we want to just leave that as config options though.
@joe-allen-89 I believe all of this text should be configurable. For example, you can update the following in course.json to indicate course progress:
We could make new string properties for when the course progress is shown on the nav bar, but that seems to overcomplicate things imo. |
@swashbuck Yes, wasn't sure if we wanted to add something similar to MCQ instruction text to avoid config issues, happy to say it's overkill and I'm making it needlessly complicated though. |
@joe-allen-89 I could see a use case where the client wants course-level progress on the nav button but a more simplified PLP drawer that only shows the current page items. I do see your point, though, and I believe @joe-replin had also mentioned having a single option to show one or the other. We already have |
@joe-allen-89 Do you mean like the |
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.
If you can see use cases of showing course in the button but page components in the drawer I think it's best to leave my queries as config options then rather than over complicating.
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.
👀
🎉 This PR is included in version 7.8.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Fixes #226
New
_useCourseProgressInNavigationButton
option to show course-level progress on the PLP nav button instead of page-level progress.Testing
_pageLevelProgress._useCourseProgressInNavigationButton
totrue
.Expected result
The PLP button in the navigation should show as partially completed since only one page is complete.