website: Switch between LTR and RTL prereq tree directions #3842

zehata · 2024-10-23T17:16:58Z

Context

Fixes #3202 Prerequisite tree right-to-left progression is confusing

Implementation

This PR extends PR #3840's implementation and adds a toggle for the user to choose whether they prefer the prerequisite to appear on the left or the right. Setting defaults to prerequisites on the left, which is the requested specifications for #3202. Please note that this branch diverges from the branch in PR #3840 and cannot both be merged.

While Issue #3203 specifications does not ask for ability to toggle, some users like myself may have already gotten used to prerequisites on the right. However, I do agree that the prerequisites on the right seems to be more intuitive.

I also took the chance to fix some confusing naming in the code, like the trees having class names that were opposite of what they contained. The fix for the truncation bug in PR #3840 is also carried over.

Credits to @sciffany for copywriting and initial implementation, to @li-kai for implementation feedback and to @alvynben for implementation and test cases.

UI tests

Settings toggle:

Single prerequisite/dependents:

Left

Right, i.e. original layout (unchanged)

Complex prerequisite/dependents with uneven layers and uneven letters in module codes

Left

Prefixed prerequisites

Side effect fixes:

This also fixes misaligned module trees as a side effect

Other Information

Any questions you may have

Is there a tele group specific for development?

Letting us know that you're new to this (so we know how much to help out)

Again, been almost 3 years since I touched any web dev. Sorry in advance if I had missed out anything.

… design

…quisite/dependent tree

…n the left

vercel · 2024-10-23T17:17:04Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
nusmods-export	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 16, 2024 10:53am

vercel · 2024-10-23T17:17:05Z

@zehata is attempting to deploy a commit to the modsbot's projects Team on Vercel.

A member of the Team first needs to authorize it.

codecov · 2024-10-23T17:20:45Z

Codecov Report

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

Project coverage is 54.81%. Comparing base (988c6fd) to head (e7d61a7).
Report is 26 commits behind head on master.

Files with missing lines	Patch %	Lines
website/src/views/settings/SettingsContainer.tsx	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3842      +/-   ##
==========================================
+ Coverage   54.52%   54.81%   +0.28%     
==========================================
  Files         274      276       +2     
  Lines        6076     6132      +56     
  Branches     1455     1470      +15     
==========================================
+ Hits         3313     3361      +48     
- Misses       2763     2771       +8

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

zehata · 2024-10-23T17:39:16Z

~~I have noted the codecov report and will work on those. However, it should not impact the implementation.~~
Fixed by 9be978c and bd7c8aa

zehata · 2024-10-26T06:51:51Z

bd7c8aa is technically outside the scope of this PR, but if it makes codecov happy then so be it.

ravern · 2024-10-30T08:44:08Z

Hi @zehata, thank you for your contribution!

I think that for backwards compatibility, we would definitely want the toggle. Can I check that means you intend for #3842 to be reviewed and to eventually just close #3840?

Let me know here and I'll review accordingly.

zehata · 2024-10-30T09:14:36Z

I think that for backwards compatibility, we would definitely want the toggle. Can I check that means you intend for #3842 to be reviewed and to eventually just close #3840?

@ravern yup! I think it would be nice to give the user a choice

…-left-to-right-prerequisites-trees

zwliew

Great job on this! Just some minor review comments and this should be good to go :)

I understand that it's been a while since you submitted the change -- would you like to continue working on this PR? If not, I could make the requested changes and merge it for you.

zwliew · 2024-12-04T09:20:12Z

website/src/reducers/settings.ts

@@ -40,6 +41,7 @@ const defaultSettingsState: SettingsState = {
  moduleTableOrder: 'exam',
  beta: false,
  loadDisqusManually: false,
+  prereqTreeOnLeft: true,


Can we retain the original behaviour and set this to false? I don't think it's a great idea to change the default.

zwliew · 2024-12-04T09:20:45Z

website/src/actions/settings.ts

@@ -92,3 +92,11 @@ export function setLoadDisqusManually(status: boolean) {
    payload: status,
  };
 }
+
+export const TOGGLE_PREREQ_TREE_DIRECTION = 'TOGGLE_PREREQ_TREE_DIRECTION' as const;
+export function togglePreReqTreeDirection(status: boolean) {


Could you rename status to something more informative like isOnLeft? It might also be better to rename togglePreReqTreeDirection to setPreReqTreeOnLeft

zwliew · 2024-12-04T09:26:25Z

website/src/views/components/ConditionalReverse.tsx

+  <>{reverse ? [...children].reverse() : children}</>
+);
+
+export default ConditionalReverse;


Let's memoize this component with React.memo

zwliew · 2024-12-04T09:36:24Z

website/src/views/modules/ModuleTree.tsx


  getModuleCondensed: (moduleCode: ModuleCode) => ModuleCondensed | undefined;
 };

 interface TreeDisplay {
  layer: number;
  node: PrereqTree;
-  isPrereq?: boolean;
+  prereqTreeOnLeft?: boolean;


Could you give prereqTreeOnLeft a default value (i.e. false) where necessary instead of leaving it as undefined? I personally don't like having unnecessarily-possibly-undefined values.

zwliew · 2024-12-04T09:36:42Z

website/src/views/modules/ModuleTree.tsx

@@ -83,39 +85,45 @@ const unwrapLayer = (node: PrereqTree) => {
 const Branch: React.FC<{
  nodes: PrereqTree[];
  layer: number;
+  prereqTreeOnLeft?: boolean;


Same comment as above above giving prereqTreeOnLeft a default value.

…-left-to-right-prerequisites-trees

zwliew

I went ahead and made the requested changes. Thanks for your contribution! Also appreciate you adding the tests

zehata · 2024-12-16T12:21:25Z

@zwliew MB, sorry about that, I was not paying attention to the PR because I thought it was exam seasons 😅

GH also didn't ping me on your comments, so I would have to sort out my notifs.

zwliew · 2024-12-16T13:42:34Z

@zwliew MB, sorry about that, I was not paying attention to the PR because I thought it was exam seasons 😅

GH also didn't ping me on your comments, so I would have to sort out my notifs.

It's all good!

zwliew · 2024-12-16T13:45:06Z

@zehata Oh also -- do join the NUSMods Telegram group that's listed on TeleNUS if you'd like to discuss development in the future :)

zehata · 2024-12-16T13:46:48Z

@zehata Oh also -- do join the NUSMods Telegram group that's listed on TeleNUS if you'd like to discuss development in the future :)

Thanks! I have already joined haha

zehata added 6 commits October 23, 2024 23:43

Added toggle in settings to change prerequisite tree direction

0fa8388

Render prerequisite tree direction according to user settings

42e67e8

Added test case for rendering prereq tree to the right, i.e. original…

65ce64f

… design

Renamed class names to reflect that direction is independent of prere…

fc75f49

…quisite/dependent tree

Modified test cases to use the default option of prerequisite trees o…

7d50120

…n the left

Fixed confusing class names that were reverse of their content

aa8e452

vercel bot deployed to Preview – nusmods-export October 23, 2024 17:17 View deployment

zehata mentioned this pull request Oct 23, 2024

Left to right prerequisites trees #3840

Closed

zehata changed the title ~~Zehata/website module page/3202/toggleable left to right prerequisites trees~~ Toggleable left to right prerequisites trees Oct 24, 2024

zehata added 2 commits October 26, 2024 14:37

Added test case for toggling prerequisite tree direction

9be978c

Added test case for prerequisite branches that require n number of mods

bd7c8aa

vercel bot deployed to Preview – nusmods-export October 26, 2024 06:51 View deployment

Merge branch 'master' into zehata/website-module-page/3202/toggleable…

768a792

…-left-to-right-prerequisites-trees

zwliew requested changes Dec 4, 2024

View reviewed changes

zwliew self-assigned this Dec 4, 2024

zwliew added 2 commits December 16, 2024 18:35

Merge branch 'master' into zehata/website-module-page/3202/toggleable…

6d72186

…-left-to-right-prerequisites-trees

Clean up code

41a538a

vercel bot deployed to Preview – nusmods-export December 16, 2024 10:44 View deployment

Fix broken test

e7d61a7

vercel bot deployed to Preview – nusmods-export December 16, 2024 10:53 View deployment

zwliew changed the title ~~Toggleable left to right prerequisites trees~~ website: Switch between LTR and RTL prereq tree directions Dec 16, 2024

zwliew approved these changes Dec 16, 2024

View reviewed changes

zwliew merged commit 65aa3cb into nusmodifications:master Dec 16, 2024
5 of 6 checks passed

zwliew mentioned this pull request Dec 17, 2024

Reverse Prerequisite Tree #3263

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

website: Switch between LTR and RTL prereq tree directions #3842

website: Switch between LTR and RTL prereq tree directions #3842

zehata commented Oct 23, 2024 •

edited

Loading

vercel bot commented Oct 23, 2024 •

edited

Loading

vercel bot commented Oct 23, 2024

codecov bot commented Oct 23, 2024 •

edited

Loading

zehata commented Oct 23, 2024 •

edited

Loading

zehata commented Oct 26, 2024

ravern commented Oct 30, 2024

zehata commented Oct 30, 2024 •

edited

Loading

zwliew left a comment •

edited

Loading

zwliew Dec 4, 2024

zwliew Dec 4, 2024

zwliew Dec 4, 2024

zwliew Dec 4, 2024 •

edited

Loading

zwliew Dec 4, 2024

zwliew left a comment •

edited

Loading

zehata commented Dec 16, 2024 •

edited

Loading

zwliew commented Dec 16, 2024

zwliew commented Dec 16, 2024

zehata commented Dec 16, 2024

website: Switch between LTR and RTL prereq tree directions #3842

website: Switch between LTR and RTL prereq tree directions #3842

Conversation

zehata commented Oct 23, 2024 • edited Loading

Context

Implementation

UI tests

Settings toggle:

Single prerequisite/dependents:

Complex prerequisite/dependents with uneven layers and uneven letters in module codes

Prefixed prerequisites

Side effect fixes:

Other Information

Any questions you may have

Letting us know that you're new to this (so we know how much to help out)

vercel bot commented Oct 23, 2024 • edited Loading

vercel bot commented Oct 23, 2024

codecov bot commented Oct 23, 2024 • edited Loading

Codecov Report

zehata commented Oct 23, 2024 • edited Loading

zehata commented Oct 26, 2024

ravern commented Oct 30, 2024

zehata commented Oct 30, 2024 • edited Loading

zwliew left a comment • edited Loading

Choose a reason for hiding this comment

zwliew Dec 4, 2024

Choose a reason for hiding this comment

zwliew Dec 4, 2024

Choose a reason for hiding this comment

zwliew Dec 4, 2024

Choose a reason for hiding this comment

zwliew Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

zwliew Dec 4, 2024

Choose a reason for hiding this comment

zwliew left a comment • edited Loading

Choose a reason for hiding this comment

zehata commented Dec 16, 2024 • edited Loading

zwliew commented Dec 16, 2024

zwliew commented Dec 16, 2024

zehata commented Dec 16, 2024

zehata commented Oct 23, 2024 •

edited

Loading

vercel bot commented Oct 23, 2024 •

edited

Loading

codecov bot commented Oct 23, 2024 •

edited

Loading

zehata commented Oct 23, 2024 •

edited

Loading

zehata commented Oct 30, 2024 •

edited

Loading

zwliew left a comment •

edited

Loading

zwliew Dec 4, 2024 •

edited

Loading

zwliew left a comment •

edited

Loading

zehata commented Dec 16, 2024 •

edited

Loading