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

feat: legacy course navigation #1239

Closed

Conversation

ArturGaspar
Copy link
Contributor

@ArturGaspar ArturGaspar commented Nov 22, 2023

Description

Add an option to enable the legacy course navigation where clicking a breadcrumb leads to the course index page highlighting the selected section.

Testing instructions

  1. Set ENABLE_LEGACY_NAV='true' and ENABLE_JUMPNAV='false'
  2. Open a course unit
  3. Click the breadcrumb navigation
  4. See that you are redirected to the course index page and that the item corresponding to the selected breadcrumb is highlighted and scrolled to

image
image
image
image

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Nov 22, 2023
@openedx-webhooks
Copy link

openedx-webhooks commented Nov 22, 2023

Thanks for the pull request, @ArturGaspar!

What's next?

Please work through the following steps to get your changes ready for engineering review:

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

  • If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
    • This process (including the steps you'll need to take) is documented here.
  • If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

  • Dependencies

    This PR must be merged before / after / at the same time as ...

  • Blockers

    This PR is waiting for OEP-1234 to be accepted.

  • Timeline information

    This PR must be merged by XX date because ...

  • Partner information

    This is for a course on edx.org.

  • Supporting documentation
  • Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

🔘 Update the status of your PR

Your PR is currently marked as a draft. After completing the steps above, update its status by clicking "Ready for Review", or removing "WIP" from the title, as appropriate.

🔘 Let us know that your PR is ready for review:

Who will review my changes?

This repository is currently maintained by @openedx/committers-frontend-app-learning. Tag them in a comment and let them know that your changes are ready for review.

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

  • The size and impact of the changes that it introduces
  • The need for product review
  • Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

@itsjeyd
Copy link

itsjeyd commented Nov 23, 2023

@ArturGaspar Thank you for this contribution! Please let me know when internal review is done and the build is green.

@jmakowski1123 This is a user-facing change so we'll most likely need a feature ticket for it. Could you please confirm?

CC @mphilbrick211

@itsjeyd itsjeyd added the product review PR requires product review before merging label Nov 23, 2023
@Agrendalath
Copy link
Member

@ArturGaspar, this needs to pass through the client's UX QA first, so we can convert the PR to a draft for now.

cc: @itsjeyd

.env.development Show resolved Hide resolved
@@ -123,6 +123,13 @@ const OutlineTab = ({ intl }) => {
}
}, [location.search]);

// Remove the initial # sign.
Copy link
Member

Choose a reason for hiding this comment

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

We should add a higher-level comment above this one to explain what we do here (and why).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Agrendalath Please let me know if it's good in the latest version now.

src/course-home/outline-tab/OutlineTab.jsx Outdated Show resolved Hide resolved
src/course-home/outline-tab/Section.jsx Outdated Show resolved Hide resolved
@@ -86,7 +87,7 @@ const SequenceLink = ({
return (
<li>
<div className={classNames('', { 'mt-2 pt-2 border-top border-light': !first })}>
<div className="row w-100 m-0">
<div className={classNames('row w-100 m-0', { 'bg-light-200': selected })}>
Copy link
Member

Choose a reason for hiding this comment

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

I recalled that the legacy approach also scrolls the page down to the highlighted unit. This is particularly useful on mobile devices. We cannot rely on adding id={id} (e.g., to the <li> tag), but we could do something like this:

const sequenceLinkRef = useRef(null);
const mutationObserver = useRef(null);

// Scroll to the selected element.
useEffect(() => {
  if (selected) {
    mutationObserver.current = new MutationObserver((mutationsList, observer) => {
      // Check if the element is already in the DOM.
      if (sequenceLinkRef.current && document.body.contains(sequenceLinkRef.current)) {
        sequenceLinkRef.current.scrollIntoView({ behavior: 'smooth' });
        // Disconnect the observer once the element is found and scrolled to.
        observer.disconnect();
      }
    });

    mutationObserver.current.observe(document.body, { childList: true, subtree: true });
  }

  // Clean up the observer on component unmount.
  return () => {
    if (mutationObserver.current) {
      mutationObserver.current.disconnect();
    }
  };
}, [selected]);

sequenceLinkRef can be added to the li element.

However, this would only work for the sequences. We should think of something that could be reused for sections and sequences.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did it without the MutationObserver and it appears to work as expected, and for both section and subsection links. Implementation is in src/course-home/outline-tab/hooks.js.

@@ -86,7 +87,7 @@ const SequenceLink = ({
return (
<li>
<div className={classNames('', { 'mt-2 pt-2 border-top border-light': !first })}>
<div className="row w-100 m-0">
<div className={classNames('row w-100 m-0', { 'bg-light-200': selected })}>
Copy link
Member

Choose a reason for hiding this comment

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

Why do we add the styles here instead of the grandparent li node?

Currently, it looks like this (I changed the color a bit to make this more apparent):
image

If we move it to li, it will look like this:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I initially did it that way because the first and last elements appear a bit taller than the others (due to the container having more padding), but I have changed this now and updated the screenshots in the PR.

src/courseware/course/CourseBreadcrumbs.jsx Outdated Show resolved Hide resolved
@ArturGaspar ArturGaspar marked this pull request as draft November 24, 2023 07:05
@ArturGaspar ArturGaspar force-pushed the artur/legacy-course-navigation branch 9 times, most recently from f158ea9 to d6a40d8 Compare November 24, 2023 16:44
Copy link

codecov bot commented Nov 24, 2023

Codecov Report

Attention: Patch coverage is 96.15385% with 1 line in your changes missing coverage. Please review.

Project coverage is 87.96%. Comparing base (bce25c4) to head (2a6b218).
Report is 219 commits behind head on master.

Files with missing lines Patch % Lines
src/index.jsx 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1239      +/-   ##
==========================================
+ Coverage   87.30%   87.96%   +0.65%     
==========================================
  Files         275      277       +2     
  Lines        4717     4743      +26     
  Branches     1190     1199       +9     
==========================================
+ Hits         4118     4172      +54     
+ Misses        580      555      -25     
+ Partials       19       16       -3     

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

@ArturGaspar ArturGaspar force-pushed the artur/legacy-course-navigation branch 8 times, most recently from 93fb125 to cbe1c51 Compare November 27, 2023 08:52
Copy link
Member

@Agrendalath Agrendalath left a comment

Choose a reason for hiding this comment

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

@ArturGaspar, looks good! Just two minor comments.

I'm not leaving a "formal" +1 on this PR yet, as we want to wait for the client's QA.

@@ -173,7 +182,11 @@ const OutlineTab = ({ intl }) => {
<Section
key={sectionId}
courseId={courseId}
defaultOpen={sections[sectionId].resumeBlock}
defaultOpen={
(selectedSectionId !== undefined)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
(selectedSectionId !== undefined)
(selectedSectionId)

Nit: it should be less error-prone if we checked all falsy values.

@import "~@edx/paragon/scss/core/core";
@import "~@edx/brand/paragon/overrides";

.section > div > div > .collapsible-body {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.section > div > div > .collapsible-body {
.section .collapsible-body {

This should be specific enough - we should not strictly rely on the Paragon's implementation.

@ArturGaspar ArturGaspar force-pushed the artur/legacy-course-navigation branch from cbe1c51 to 9a04514 Compare November 27, 2023 13:27
@ArturGaspar ArturGaspar force-pushed the artur/legacy-course-navigation branch from 9a04514 to 1f092c8 Compare December 8, 2023 19:07
Add an option to enable the legacy course navigation where clicking a
breadcrumb leads to the course index page highlighting the selected section.
@ArturGaspar ArturGaspar marked this pull request as ready for review December 8, 2023 19:35
@jmakowski1123
Copy link

There is a larger project in play now to bring back the sidebar navigation experience, which was the original experience that was replaced by the breadcrumbs. Part of the consideration in that project is how many navigation options we need, and the breadcrumb pathway is being reconsidered in that context. We need to come to a decision first about which navigation experience the core product supports as the primary one, before investing in this. Suggest to put this on temporarily on hold. https://openedx.atlassian.net/wiki/spaces/OEPM/pages/3909779457/Feature+Enhancement+Proposal+Restore+the+left-sidebar+navigation+in+the+learner+experience

@itsjeyd itsjeyd added the blocked by other work PR cannot be finished until other work is complete label Dec 14, 2023
@Agrendalath
Copy link
Member

@jmakowski1123, the project you mentioned sounds like a significant change. Do you know if there is any timeline for making this decision?

This PR brings back the legacy (non-MFE) experience - it only changes the action triggered by clicking on the text in the course navigation. If we decide to delete the breadcrumbs, the scope of that project won't be altered in any way by this change.
In the meantime, this PR gives us a (rather nice) side effect of highlighting specific parts of the Course Outline when redirecting users from external pages. Let's say we add a link like LMS_URL/course/{course_name}#{block_id} to the external page. When users click on it, they will be directed to the Course Outline page with the highlighted block_id (section/subsection), as shown on the screenshots. This is especially useful for the mobile experience, as the page is automatically scrolled to the highlighted element of the Course Outline.

cc: @itsjeyd

@jmakowski1123
Copy link

@jmakowski1123, the project you mentioned sounds like a significant change. Do you know if there is any timeline for making this decision?

This PR brings back the legacy (non-MFE) experience - it only changes the action triggered by clicking on the text in the course navigation. If we decide to delete the breadcrumbs, the scope of that project won't be altered in any way by this change. In the meantime, this PR gives us a (rather nice) side effect of highlighting specific parts of the Course Outline when redirecting users from external pages. Let's say we add a link like LMS_URL/course/{course_name}#{block_id} to the external page. When users click on it, they will be directed to the Course Outline page with the highlighted block_id (section/subsection), as shown on the screenshots. This is especially useful for the mobile experience, as the page is automatically scrolled to the highlighted element of the Course Outline.

cc: @itsjeyd

I'd like to push for agreement/decision by the end of Jan, with the goal of beginning implementation in Feb for hopeful inclusion in Redwood. You can follow, and join, the conversation here, please feel free to join the wiki thread: https://openedx.atlassian.net/wiki/spaces/OEPM/pages/3909779457/Feature+Enhancement+Proposal+Restore+the+left-sidebar+navigation+in+the+learner+experience

xitij2000

This comment was marked as off-topic.

@itsjeyd
Copy link

itsjeyd commented Feb 16, 2024

I'd like to push for agreement/decision by the end of Jan, with the goal of beginning implementation in Feb for hopeful inclusion in Redwood. You can follow, and join, the conversation here, please feel free to join the wiki thread: openedx.atlassian.net/wiki/spaces/OEPM/pages/3909779457/Feature+Enhancement+Proposal+Restore+the+left-sidebar+navigation+in+the+learner+experience

Hey @jmakowski1123, based on a brief look at the wiki thread it seems like product review is still in progress. Do you have any updates about the timeline for this work?

CC @Agrendalath @ArturGaspar

@itsjeyd
Copy link

itsjeyd commented Mar 11, 2024

Hey @cassiezamparini, would you have time to help get the ball rolling again here in terms of product review?

@cassiezamparini
Copy link

@itsjeyd Sure thing! I'll add this to my to-do list for tomorrow :)

@cassiezamparini
Copy link

cassiezamparini commented Mar 12, 2024

@itsjeyd It looks like this feature is on hold based on the comments from @jmakowski1123 in this thread. But I see you linked to my comment on the sidebar query. I'll follow up on this on the Wiki.

Also in order to do a product review, we need to follow the new process @ArturGaspar @Agrendalath - as outlined here. Please review the link above and follow the steps I mention below:

  • Create product proposal using a Template: Choose Your Template - Open Source Product Contributions
  • Create a Product Proposal ticket in the platform-roadmap GitHub repository (with a link to the product proposal etc). Note, tickets must be created here. The ticket will get auto-added to the Open edX Roadmap project board: Open edX Roadmap • openedx
  • The Submitter pings the Core Product Working Group in Slack (#wg-product-core) and shares the proposal. You can ping me once you've created the proposal and I can ping the Core Product WG on your behalf.

Please let me know if anything is unclear. We are still in the process of ironing out this new product review process, so your feedback will be welcomed.

@cassiezamparini
Copy link

@itsjeyd @Agrendalath @ArturGaspar

In relation to Feature Enhancement Proposal: Restore the left-sidebar navigation in the learner experience:

Edward Zarecor:
Yes, the design phase has wrapped up and dev phase is starting now. It's targeted for GA in Redwood.

Thoughts?

@itsjeyd
Copy link

itsjeyd commented Mar 22, 2024

@cassiezamparini Thanks for looking into the status of product review for this PR!

Edward Zarecor:
Yes, the design phase has wrapped up and dev phase is starting now. It's targeted for GA in Redwood.

Thoughts?

I don't know what GA means, and I'm not sure how to proceed from here.

If the dev phase is starting now, does that mean we can consider this PR approved from the product perspective?

--

Either way, it's probably worth noting that my involvement here is limited to OSPR management (which currently doesn't include tracking individual steps of the product review process) 🙂

From that perspective, the main question to answer is the one above, i.e. whether product review is done and we can move forward with finding engineering reviewers for this PR.

--

CC @ArturGaspar @Agrendalath @e0d

@itsjeyd
Copy link

itsjeyd commented Mar 22, 2024

Internal-Ref: https://tasks.opencraft.com/browse/BB-8160

@jmakowski1123
Copy link

jmakowski1123 commented Mar 25, 2024

As part of the project to reimplement the left sidebar navigation (full MVP spec here), I do not think it is necessary to invest in any legacy navigation experiences. The project to reimplement the left sidebar nav is a separate project from this proposal, and I think it will be possible to close this proposal as a result of the other project.

I'm assuming the point of proposing to revive the legacy breadcrumb experience is to make it easier for learners to see where they are in the course outline? This problem will be solved when we reimplement the left sidebar, as learners will be able to wayfind through the course outline without leaving the content.

The left sidebar will be revived for the Redwood release in June and will be the default experience out of the box. @ArturGaspar does this meet your needs, and if so, can we close this proposal?

@itsjeyd itsjeyd added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed blocked by other work PR cannot be finished until other work is complete labels Apr 12, 2024
@ArturGaspar ArturGaspar marked this pull request as draft April 19, 2024 14:40
@ArturGaspar
Copy link
Contributor Author

@jmakowski1123 It is possible that the sidebar navigation would fulfil our needs, so I am converting this PR to a draft until the released version can be evaluated. Thank you.

@itsjeyd itsjeyd added blocked by other work PR cannot be finished until other work is complete and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Apr 22, 2024
@itsjeyd
Copy link

itsjeyd commented Sep 7, 2024

Hey @ArturGaspar and @Agrendalath, just checking in:

It is possible that the sidebar navigation would fulfil our needs, so I am converting this PR to a draft until the released version can be evaluated.

Do you have any updates on when this evaluation would happen?

@Agrendalath
Copy link
Member

@itsjeyd, it's in progress. We should have more info within the next two months.

cc: @ArturGaspar

@itsjeyd
Copy link

itsjeyd commented Sep 20, 2024

Sounds good, thanks @Agrendalath.

@itsjeyd itsjeyd removed the blocked by other work PR cannot be finished until other work is complete label Sep 20, 2024
@itsjeyd
Copy link

itsjeyd commented Nov 7, 2024

@Agrendalath It's been two months now, so checking in to see if you have any updates to share :)

@Agrendalath
Copy link
Member

@itsjeyd, we haven't gotten a clear decision about this yet. It may take another month or two.

@Agrendalath
Copy link
Member

We will use the navigation sidebar. Closing.

@Agrendalath Agrendalath deleted the artur/legacy-course-navigation branch December 19, 2024 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U product review PR requires product review before merging
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants