Skip to content

feat: use navigation sequence metadata to disable navigation components - #1273

Merged
arbrandes merged 2 commits into
openedx:masterfrom
eduNEXT:MJG/hide-from-sequence-nav
Mar 8, 2024
Merged

feat: use navigation sequence metadata to disable navigation components#1273
arbrandes merged 2 commits into
openedx:masterfrom
eduNEXT:MJG/hide-from-sequence-nav

Conversation

@mariajgrimaldi

@mariajgrimaldi mariajgrimaldi commented Jan 12, 2024

Copy link
Copy Markdown
Member

Settings

EDX_PLATFORM_REPOSITORY: https://github.com/eduNEXT/edx-platform.git
EDX_PLATFORM_VERSION: MJG/hide-from-navigation-toc

TUTOR_GROVE_CMS_ENV_FEATURES: |
  ENABLE_HIDE_FROM_TOC_UI: True

Description

This PR adds a flag check to navigation components (next/previous buttons & course sequence breadcrumbs) to disable them if navigation_disabled is True.

The navigation_disabled field comes from this edx-platform PR: openedx/openedx-platform#34049, which returns the new field as sequence metadata, based on whether the sequence was configured as Hide from TOC. This kind of flag could also be extended for other use cases, not just hidden from TOC.

These changes are part of a series of PR(s) that implement the Feature Enhancement Proposal: Hide Sections from course outline:
https://github.com/openedx/edx-platform/pulls?q=is%3Apr+is%3Aopen+hide+from+toc
https://github.com/openedx/frontend-app-learning/pulls?q=is%3Apr+is%3Aopen+hide+from+toc

How to test

Test setup using tutor:

  1. git clone https://github.com/openedx/frontend-app-learning.git
  2. git fetch origin pull/1273/head:hide-from-sequence-nav
  3. Then, run: tutor mounts add ./frontend-app-learning
  4. cd frontend-app-learning && npm install, ensure you're using node 18
  5. tutor dev launch

It's better to test this out along with this PR: openedx/openedx-platform#33952. So first, follow the instructions in that PR. Don't change branches; apply these commits on top of the branch.

For the Learning MFE changes, please follow the instructions listed here. Then, go to the LMS using the link to your hidden subsection, it should look something like this with > 1 and exactly 1:

Screencast.from.13-02-24.12.55.13.mp4

With 1 unit it doesn't show prev, next and sequence breadcrumbs. With more than one, it shows next and previous for units within the same sequence.

Other information

As I mentioned, this PR is part of a series of PRs implementing the feature enhancement of Hide From TOC. This initiative is an open-source contribution to the Open edX platform funded by a Unidigital project from the Spanish Government - 2023.

@openedx-webhooks

openedx-webhooks commented Jan 12, 2024

Copy link
Copy Markdown

Thanks for the pull request, @mariajgrimaldi! 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:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

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.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jan 12, 2024
@mariajgrimaldi mariajgrimaldi changed the title feat: use navigation sequence metadata to disabled prev/next buttons feat: use navigation sequence metadata to disable navigation components Jan 12, 2024
@codecov

codecov Bot commented Jan 12, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.40%. Comparing base (7c8e26d) to head (523ab85).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1273      +/-   ##
==========================================
+ Coverage   88.38%   88.40%   +0.01%     
==========================================
  Files         291      291              
  Lines        4951     4959       +8     
  Branches     1253     1262       +9     
==========================================
+ Hits         4376     4384       +8     
  Misses        561      561              
  Partials       14       14              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mariajgrimaldi
mariajgrimaldi marked this pull request as ready for review January 12, 2024 22:57
Comment thread src/courseware/course/Course.jsx Outdated
@mariajgrimaldi
mariajgrimaldi force-pushed the MJG/hide-from-sequence-nav branch from 44071a4 to d966fc6 Compare January 16, 2024 16:01
@itsjeyd itsjeyd added blocked by other work PR cannot be finished until other work is complete product review PR requires product review before merging labels Jan 18, 2024
@itsjeyd

itsjeyd commented Jan 18, 2024

Copy link
Copy Markdown

Hi @mariajgrimaldi, thank you for this contribution!

Like #1262, it seems to relate to openedx/platform-roadmap#301, so I'm flagging it for product review.

CC @jmakowski1123

@itsjeyd itsjeyd added the core contributor PR author is a Core Contributor (who may or may not have write access to this repo). label Jan 18, 2024
Comment thread src/courseware/course/Course.jsx
Comment thread src/courseware/course/sequence/sequence-navigation/SequenceNavigation.jsx Outdated
Comment thread src/courseware/course/sequence/sequence-navigation/hooks.js Outdated
Comment thread src/courseware/course/sequence/sequence-navigation/hooks.js Outdated
@santiagosuarezedunext

Copy link
Copy Markdown

Hi Tim, this has already gone through product review, it was discussed and approved in this post. greetings!
CC: @jmakowski1123

@mariajgrimaldi
mariajgrimaldi force-pushed the MJG/hide-from-sequence-nav branch from d966fc6 to 7f173bb Compare February 13, 2024 13:07

@johnvente johnvente left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM! thanks

@mariajgrimaldi
mariajgrimaldi force-pushed the MJG/hide-from-sequence-nav branch from 7f173bb to 203a5e3 Compare February 13, 2024 15:34
@BryanttV

Copy link
Copy Markdown
Contributor

Hi @mariajgrimaldi, I tested this PR in my local and it works correctly!

@itsjeyd itsjeyd added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Feb 22, 2024
@itsjeyd

itsjeyd commented Feb 22, 2024

Copy link
Copy Markdown

Hey @mariajgrimaldi, aside from fixing conflicts this still needs approval from a CC and/or maintainer, right?

@mariajgrimaldi

Copy link
Copy Markdown
Member Author

Sorry for the delay, @itsjeyd, yes! We still need the maintainers' approval.

@arbrandes

Copy link
Copy Markdown
Contributor

I'll bite. Let's see if we can get this working in a PR sandbox, first.

@arbrandes
arbrandes self-requested a review March 1, 2024 18:01
@arbrandes arbrandes added the create-sandbox open-craft-grove should create a sandbox environment from this PR label Mar 1, 2024
@mariajgrimaldi

mariajgrimaldi commented Mar 1, 2024

Copy link
Copy Markdown
Member Author

@arbrandes: this needs openedx/openedx-platform#34049 to work. Is there a way to configure it? If we can't configure it, I'll let you know the PR is merged.

@arbrandes

arbrandes commented Mar 1, 2024

Copy link
Copy Markdown
Contributor

Yes, there is! I just did (see the Settings section at the top of the PR description). Let's see if it works. It'll take up to an hour for the sandbox to come up.

@open-craft-grove

Copy link
Copy Markdown

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@arbrandes

Copy link
Copy Markdown
Contributor

I've tested this in the sandbox, and it works! Now, to look at the code.

@arbrandes arbrandes left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just a couple of changes, otherwise looks and works great!

Comment thread src/courseware/course/Course.jsx Outdated
Comment thread src/courseware/course/sequence/sequence-navigation/SequenceNavigation.jsx Outdated
Comment thread src/courseware/course/sequence/sequence-navigation/SequenceNavigation.jsx Outdated
Comment thread src/courseware/course/sequence/sequence-navigation/hooks.js Outdated
@mariajgrimaldi
mariajgrimaldi force-pushed the MJG/hide-from-sequence-nav branch from 694269d to 523ab85 Compare March 7, 2024 18:47
@mariajgrimaldi

Copy link
Copy Markdown
Member Author

@arbrandes: comments addressed. Thank you!

@open-craft-grove

Copy link
Copy Markdown

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@arbrandes
arbrandes merged commit bca3aac into openedx:master Mar 8, 2024
@openedx-webhooks

Copy link
Copy Markdown

@mariajgrimaldi 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

@itsjeyd itsjeyd removed blocked by other work PR cannot be finished until other work is complete waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core contributor PR author is a Core Contributor (who may or may not have write access to this repo). create-sandbox open-craft-grove should create a sandbox environment from this PR open-source-contribution PR author is not from Axim or 2U product review PR requires product review before merging

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

8 participants