feat: legacy course navigation #1239

ArturGaspar · 2023-11-22T15:10:20Z

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

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

openedx-webhooks · 2023-11-22T15:10:26Z

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

This is currently a draft pull request. When it is ready for our review and all tests are green, click "Ready for Review", or remove "WIP" from the title, as appropriate.

itsjeyd · 2023-11-23T13:58:26Z

@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

Agrendalath · 2023-11-23T16:33:50Z

@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

Agrendalath · 2023-11-23T22:12:46Z

src/course-home/outline-tab/OutlineTab.jsx

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

+  // Remove the initial # sign.


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

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

src/course-home/outline-tab/OutlineTab.jsx

src/course-home/outline-tab/Section.jsx

Agrendalath · 2023-11-23T22:24:17Z

src/course-home/outline-tab/SequenceLink.jsx

@@ -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 })}>


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.

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.

Agrendalath · 2023-11-23T22:24:30Z

src/course-home/outline-tab/SequenceLink.jsx

@@ -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 })}>


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):

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

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

codecov · 2023-11-24T16:52:04Z

Codecov Report

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

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

Files	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.

Agrendalath

@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.

Agrendalath · 2023-11-27T12:20:29Z

src/course-home/outline-tab/OutlineTab.jsx

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


Suggested change

(selectedSectionId !== undefined)

(selectedSectionId)

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

Agrendalath · 2023-11-27T12:33:23Z

src/course-home/outline-tab/Section.scss

+@import "~@edx/paragon/scss/core/core";
+@import "~@edx/brand/paragon/overrides";
+
+.section > div > div > .collapsible-body {


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.

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

Agrendalath

👍

I tested this: checked that the legacy course navigation works as expected
I read through the code
I checked for accessibility issues: n/a
Includes documentation: n/a

Agrendalath · 2023-12-08T19:34:59Z

@ArturGaspar, please mark this PR as ready for review.

I left one review comment on the backport PR (open-craft#6 (comment)) instead of this one. I'm mentioning this for the full context

jmakowski1123 · 2023-12-14T01:17:45Z

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

Agrendalath · 2023-12-15T14:46:51Z

@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 · 2023-12-21T19:11:29Z

@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

itsjeyd · 2024-02-16T13:56:25Z

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 · 2024-03-11T08:44:58Z

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

cassiezamparini · 2024-03-11T08:49:45Z

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

cassiezamparini · 2024-03-12T10:28:08Z

@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 · 2024-03-12T14:52:30Z

@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 · 2024-03-22T09:03:28Z

@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 · 2024-03-22T09:06:44Z

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

jmakowski1123 · 2024-03-25T20:58:20Z

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?

ArturGaspar · 2024-04-19T14:41:43Z

@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.

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

ArturGaspar mentioned this pull request Nov 22, 2023

feat: legacy course navigation open-craft/frontend-app-learning#6

Merged

itsjeyd added the product review PR requires product review before merging label Nov 23, 2023

Agrendalath suggested changes Nov 23, 2023

View reviewed changes

ArturGaspar marked this pull request as draft November 24, 2023 07:05

ArturGaspar force-pushed the artur/legacy-course-navigation branch 9 times, most recently from f158ea9 to d6a40d8 Compare November 24, 2023 16:44

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

Agrendalath reviewed Nov 27, 2023

View reviewed changes

ArturGaspar force-pushed the artur/legacy-course-navigation branch from cbe1c51 to 9a04514 Compare November 27, 2023 13:27

ArturGaspar force-pushed the artur/legacy-course-navigation branch from 9a04514 to 1f092c8 Compare December 8, 2023 19:07

feat: legacy course navigation

2a6b218

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

ArturGaspar force-pushed the artur/legacy-course-navigation branch from 1f092c8 to 2a6b218 Compare December 8, 2023 19:13

Agrendalath approved these changes Dec 8, 2023

View reviewed changes

ArturGaspar marked this pull request as ready for review December 8, 2023 19:35

itsjeyd added the blocked by other work PR cannot be finished until other work is complete label Dec 14, 2023

0x29a mentioned this pull request Jan 4, 2024

feat: port code drift for open-craft/frontend-app-learning [BB-8367] open-craft/frontend-app-learning#7

Merged

This comment was marked as off-topic.

Sign in to view

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 marked this pull request as draft April 19, 2024 14:40

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: legacy course navigation #1239

feat: legacy course navigation #1239

ArturGaspar commented Nov 22, 2023 •

edited

openedx-webhooks commented Nov 22, 2023 •

edited

itsjeyd commented Nov 23, 2023

Agrendalath commented Nov 23, 2023

Agrendalath Nov 23, 2023

ArturGaspar Nov 24, 2023

Agrendalath Nov 23, 2023

ArturGaspar Nov 27, 2023

Agrendalath Nov 23, 2023

ArturGaspar Nov 24, 2023

codecov bot commented Nov 24, 2023 •

edited

Agrendalath left a comment

Agrendalath Nov 27, 2023

Agrendalath Nov 27, 2023

Agrendalath left a comment

Agrendalath commented Dec 8, 2023 •

edited

jmakowski1123 commented Dec 14, 2023

Agrendalath commented Dec 15, 2023

jmakowski1123 commented Dec 21, 2023

This comment was marked as off-topic.

itsjeyd commented Feb 16, 2024

itsjeyd commented Mar 11, 2024

cassiezamparini commented Mar 11, 2024

cassiezamparini commented Mar 12, 2024 •

edited

cassiezamparini commented Mar 12, 2024

itsjeyd commented Mar 22, 2024

itsjeyd commented Mar 22, 2024

jmakowski1123 commented Mar 25, 2024 •

edited

ArturGaspar commented Apr 19, 2024

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

feat: legacy course navigation #1239

Are you sure you want to change the base?

feat: legacy course navigation #1239

Conversation

ArturGaspar commented Nov 22, 2023 • edited

Description

Testing instructions

openedx-webhooks commented Nov 22, 2023 • edited

itsjeyd commented Nov 23, 2023

Agrendalath commented Nov 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 24, 2023 • edited

Codecov Report

Agrendalath left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Agrendalath left a comment

Choose a reason for hiding this comment

Agrendalath commented Dec 8, 2023 • edited

jmakowski1123 commented Dec 14, 2023

Agrendalath commented Dec 15, 2023

jmakowski1123 commented Dec 21, 2023

This comment was marked as off-topic.

itsjeyd commented Feb 16, 2024

itsjeyd commented Mar 11, 2024

cassiezamparini commented Mar 11, 2024

cassiezamparini commented Mar 12, 2024 • edited

cassiezamparini commented Mar 12, 2024

itsjeyd commented Mar 22, 2024

itsjeyd commented Mar 22, 2024

jmakowski1123 commented Mar 25, 2024 • edited

ArturGaspar commented Apr 19, 2024

ArturGaspar commented Nov 22, 2023 •

edited

openedx-webhooks commented Nov 22, 2023 •

edited

codecov bot commented Nov 24, 2023 •

edited

Agrendalath commented Dec 8, 2023 •

edited

cassiezamparini commented Mar 12, 2024 •

edited

jmakowski1123 commented Mar 25, 2024 •

edited