fix: make nav buttons use links for accessibility #1137

mohd-akram · 2023-07-07T07:57:17Z

Closes #753

Testing instructions

Run npm test
Test the following:
- Previous and Next buttons in the sequence bar
- Previous and Next buttons in the unit
- Unit buttons in the sequence bar
- Both Previous buttons are disabled in first unit of first section
- Both Next buttons are disabled in last unit of last section
- Next button in last unit of last section is enabled and goes to course exit if that is configured

openedx-webhooks · 2023-07-07T07:57:22Z

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

Agrendalath · 2023-07-07T11:44:48Z

@BbrSofiane, @brian-smith-tcril, would you mind triggering the CI?

mohd-akram · 2023-07-07T14:19:27Z

It didn't seem to run the tests till the end. Failed midway with this:

Pact Binary Error: Could not load existing consumer contract from /home/runner/work/frontend-app-learning/frontend-app-learning/src/pacts/frontend-app-learning-lms.json due to undefined method `key?' for nil:NilClass. Creating a new file.

farhaanbukhsh · 2023-07-10T08:29:47Z

src/courseware/course/sequence/sequence-navigation/hooks.js

+  let nextLink;
+  if (isLastUnit) {
+    nextLink = `/course/${courseId}/course-end`;
+  } else {
+    const nextIndex = unitIndex + 1;
+    if (nextIndex < sequence.unitIds.length) {
+      const nextUnitId = sequence.unitIds[nextIndex];
+      nextLink = `/course/${courseId}/${currentSequenceId}/${nextUnitId}`;
+    } else if (nextSequenceId) {
+      nextLink = `/course/${courseId}/${nextSequenceId}/first`;
+    }
+  }
+
+  let previousLink;
+  const previousIndex = unitIndex - 1;
+  if (previousIndex >= 0) {
+    const previousUnitId = sequence.unitIds[previousIndex];
+    previousLink = `/course/${courseId}/${currentSequenceId}/${previousUnitId}`;
+  } else if (previousSequenceId) {
+    previousLink = `/course/${courseId}/${previousSequenceId}/last`;
+  }


We can probably have two functions, getNextLink, getPreviousLink, and the logic can be moved into them, it will help to maintain the code and to find them. What do you think?

We could, but it will duplicate some logic (eg. isLastUnit). I'm also not sure where I'd put them.

I think you can create a series of interdependent hooks.

i.e. useNextLink, and usePreviousLink, that can both use common hooks. i.e. they can both pull in use a common hook that returns isLastUnit

0x29a

👍

I tested this: all buttons work and show as expected.
I read through the code.
This MFE is now more accessible. 🙂

src/courseware/CoursewareContainer.jsx

src/courseware/course/sequence/Sequence.jsx

mohd-akram · 2023-07-11T07:16:08Z

It didn't seem to run the tests till the end. Failed midway with this:

Pact Binary Error: Could not load existing consumer contract from /home/runner/work/frontend-app-learning/frontend-app-learning/src/pacts/frontend-app-learning-lms.json due to undefined method `key?' for nil:NilClass. Creating a new file.

If anyone has any idea how to fix this or if it's just an intermittent error, re-run the tests, that would be appreciated.

EDIT: I've verified that this is an intermittent error and the tests should pass.

0x29a · 2023-07-11T11:13:17Z

@BbrSofiane, @brian-smith-tcril, could you please trigger the CI again?

itsjeyd · 2023-07-11T15:41:01Z

@mohd-akram Thank you for this contribution!

codecov · 2023-07-12T16:57:27Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.21% 🎉

Comparison is base (e95a59c) 87.67% compared to head (3a96fcb) 87.89%.
Report is 7 commits behind head on master.

❗ Current head 3a96fcb differs from pull request most recent head 8890fe0. Consider uploading reports for the commit 8890fe0 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1137      +/-   ##
==========================================
+ Coverage   87.67%   87.89%   +0.21%     
==========================================
  Files         264      264              
  Lines        4455     4477      +22     
  Branches     1113     1123      +10     
==========================================
+ Hits         3906     3935      +29     
+ Misses        535      528       -7     
  Partials       14       14

Files Changed	Coverage Δ
src/courseware/CoursewareContainer.jsx	`84.45% <100.00%> (+2.10%)`	⬆️
src/courseware/course/sequence/Sequence.jsx	`98.76% <100.00%> (+3.47%)`	⬆️
...equence/sequence-navigation/SequenceNavigation.jsx	`100.00% <100.00%> (ø)`
...course/sequence/sequence-navigation/UnitButton.jsx	`94.44% <100.00%> (+0.32%)`	⬆️
...se/sequence/sequence-navigation/UnitNavigation.jsx	`100.00% <100.00%> (ø)`
...eware/course/sequence/sequence-navigation/hooks.js	`100.00% <100.00%> (ø)`

... and 3 files with indirect coverage changes

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

Agrendalath

👍

I tested this: tested the navigation locally
I read through the code
I checked for accessibility issues
Includes documentation: n/a
I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository: n/a

Agrendalath · 2023-07-12T19:17:14Z

@BbrSofiane, @brian-smith-tcril, would you like to review this as CC? It closes an issue reported in this repo (#753).

itsjeyd · 2023-07-18T07:41:00Z

@mattcarter FYI^^

Please let us know if you're planning to have someone from the Aurora team look at these changes as well.

itsjeyd · 2023-07-26T08:27:13Z

Hey @BbrSofiane and @brian-smith-tcril, friendly ping about @Agrendalath's question above.

CC @mattcarter

brian-smith-tcril

Thank you so much for this! I left a few comments with questions, but overall this is looking great!

src/courseware/course/Course.test.jsx

src/courseware/course/sequence/Sequence.jsx

brian-smith-tcril · 2023-08-01T18:22:04Z

src/courseware/course/sequence/sequence-navigation/SequenceNavigation.jsx

@@ -1,4 +1,5 @@
 import React from 'react';
+import { Link } from 'react-router-dom';


Is there a reason we aren't already using these? This feels like it simplifies the implementation quite a bit (which I'm all for!). I'm worried I don't have enough context on the original implementation to know if that was an intentional architectural decision or not. I'm just not sure if there's something I'm missing that prevented use of react-router links in the first place.

No idea. I can see that react router links are used in some other places (eg. CourseBreadcrumbs).

itsjeyd · 2023-08-08T08:34:53Z

@mohd-akram Looks like this is getting close to merging! If you could fix the remaining commitlint failure, we should get a green build here.

CC @brian-smith-tcril

mohd-akram · 2023-08-08T09:53:25Z

Fixed.

muselesscreator

From high level, LGTM

openedx-webhooks · 2023-08-14T17:18:59Z

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

kdmccormick · 2023-08-14T17:23:39Z

Thanks @mohd-akram ! 🎉

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

farhaanbukhsh reviewed Jul 10, 2023

View reviewed changes

0x29a approved these changes Jul 10, 2023

View reviewed changes

src/courseware/CoursewareContainer.jsx Show resolved Hide resolved

src/courseware/course/sequence/Sequence.jsx Show resolved Hide resolved

src/courseware/course/sequence/Sequence.jsx Show resolved Hide resolved

itsjeyd added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Jul 11, 2023

Agrendalath approved these changes Jul 12, 2023

View reviewed changes

e0d removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Jul 14, 2023

itsjeyd added the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label Jul 18, 2023

brian-smith-tcril reviewed Aug 1, 2023

View reviewed changes

itsjeyd added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed waiting for eng review PR is ready for review. Review and merge it, or suggest changes. labels Aug 8, 2023

fix: make nav buttons use links for accessibility

8890fe0

muselesscreator approved these changes Aug 8, 2023

View reviewed changes

mattcarter merged commit 5ee6190 into openedx:master Aug 14, 2023
5 checks passed

mohd-akram deleted the nav-links branch August 22, 2023 11:08

itsjeyd removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Sep 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: make nav buttons use links for accessibility #1137

fix: make nav buttons use links for accessibility #1137

mohd-akram commented Jul 7, 2023

openedx-webhooks commented Jul 7, 2023

Agrendalath commented Jul 7, 2023

mohd-akram commented Jul 7, 2023

farhaanbukhsh Jul 10, 2023

mohd-akram Jul 11, 2023

xitij2000 Jul 17, 2023

0x29a left a comment

mohd-akram commented Jul 11, 2023 •

edited

Loading

0x29a commented Jul 11, 2023

itsjeyd commented Jul 11, 2023

codecov bot commented Jul 12, 2023 •

edited

Loading

Agrendalath left a comment

Agrendalath commented Jul 12, 2023

itsjeyd commented Jul 18, 2023

itsjeyd commented Jul 26, 2023

brian-smith-tcril left a comment

brian-smith-tcril Aug 1, 2023

mohd-akram Aug 2, 2023

itsjeyd commented Aug 8, 2023

mohd-akram commented Aug 8, 2023

muselesscreator left a comment

openedx-webhooks commented Aug 14, 2023

kdmccormick commented Aug 14, 2023

		@@ -1,4 +1,5 @@
		import React from 'react';
		import { Link } from 'react-router-dom';

fix: make nav buttons use links for accessibility #1137

fix: make nav buttons use links for accessibility #1137

Conversation

mohd-akram commented Jul 7, 2023

Testing instructions

openedx-webhooks commented Jul 7, 2023

Agrendalath commented Jul 7, 2023

mohd-akram commented Jul 7, 2023

farhaanbukhsh Jul 10, 2023

Choose a reason for hiding this comment

mohd-akram Jul 11, 2023

Choose a reason for hiding this comment

xitij2000 Jul 17, 2023

Choose a reason for hiding this comment

0x29a left a comment

Choose a reason for hiding this comment

mohd-akram commented Jul 11, 2023 • edited Loading

0x29a commented Jul 11, 2023

itsjeyd commented Jul 11, 2023

codecov bot commented Jul 12, 2023 • edited Loading

Codecov Report

Agrendalath left a comment

Choose a reason for hiding this comment

Agrendalath commented Jul 12, 2023

itsjeyd commented Jul 18, 2023

itsjeyd commented Jul 26, 2023

brian-smith-tcril left a comment

Choose a reason for hiding this comment

brian-smith-tcril Aug 1, 2023

Choose a reason for hiding this comment

mohd-akram Aug 2, 2023

Choose a reason for hiding this comment

itsjeyd commented Aug 8, 2023

mohd-akram commented Aug 8, 2023

muselesscreator left a comment

Choose a reason for hiding this comment

openedx-webhooks commented Aug 14, 2023

kdmccormick commented Aug 14, 2023

mohd-akram commented Jul 11, 2023 •

edited

Loading

codecov bot commented Jul 12, 2023 •

edited

Loading