-
Notifications
You must be signed in to change notification settings - Fork 80
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
feat: course outline page #694
feat: course outline page #694
Conversation
Thanks for the pull request, @navinkarkera! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #694 +/- ##
==========================================
+ Coverage 88.65% 88.85% +0.20%
==========================================
Files 447 468 +21
Lines 6846 7168 +322
Branches 1461 1539 +78
==========================================
+ Hits 6069 6369 +300
- Misses 752 772 +20
- Partials 25 27 +2 ☔ View full report in Codecov by Sentry. |
2f3b953
to
543b171
Compare
1fe7488
to
ded2c81
Compare
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.
👍 Some minor suggestions, but otherwise LGTM.
- I tested this
- I read through the code
- [-] I checked for accessibility issues
- [-] Includes documentation
src/course-outline/header-navigations/HeaderNavigation.test.jsx
Outdated
Show resolved
Hide resolved
@Cup0fCoffee Thanks for the thorough review. Addressed all your comments. |
I've created issue TNL-11237 in the private 2U Jira. |
7ce8fa8
to
5ddb9bf
Compare
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.
Just need to make the one message change that I previously requested. After that I will merge this PR.
* feat: [2u-259] add components * feat: [2u-259] fix sidebar * feat: [2u-259] add tests, fix links * feat: [2u-259] fix messages * feat: [2u-159] fix reducer and sidebar * feat: [2u-259] fix reducer * feat: [2u-259] remove warning from selectors * feat: [2u-259] remove indents --------- Co-authored-by: Vladislav Keblysh <[email protected]> feat: Course outline Status Bar (#50) * feat: [2u-259] add components * feat: [2u-259] fix sidebar * feat: [2u-259] add tests, fix links * feat: [2u-259] fix messages * feat: [2u-159] fix reducer and sidebar * feat: add checklist * feat: [2u-259] fix reducer * feat: [2u-259] remove warning from selectors * feat: [2u-259] remove indents * feat: [2u-259] add api, enable modal * feat: [2u-259] add tests * feat: [2u-259] add translates * feat: [2u-271] fix transalates * feat: [2u-281] fix isQuery pending, utils, hooks * feat: [2u-281] fix useScrollToHashElement * feat: [2u-271] fix imports --------- Co-authored-by: Vladislav Keblysh <[email protected]> feat: Course Outline Reindex (#55) * feat: [2u-277] add alerts * feat: [2u-277] add translates * feat: [2u-277] fix tests * fix: [2u-277] fix slice and hook --------- Co-authored-by: Vladislav Keblysh <[email protected]> fix: Course outline tests (#56) * fix: fixed course outline status bar tests * fix: fixed course outline status bar tests * fix: fixed course outline enable highlights modal tests * fix: enable modal tests fix: increase code coverage on the page
feat: lms live link chore: update outline link fix: course outline link refactor: remove unnecessary css and rename test file refactor: remove unnecessary css from outlineSidebar test: make use of message variable instead of hardcoded text refactor: remove unnecessary h5 class test: use test id for detecting component refactor: update course outline url and some default messages
2875840
to
80cc5e3
Compare
@KristinAoki Done. Also squashed commits, this is ready to be merged. |
@navinkarkera 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Continuing work from #598
This PR adds
Screenshot:
Depends on: openedx/edx-platform#33667
Testing instructions: