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

Toggleable left to right prerequisites trees #3842

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

zehata
Copy link

@zehata zehata commented Oct 23, 2024

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:

image

Single prerequisite/dependents:

Left
image

Right, i.e. original layout (unchanged)
image

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

Left
image

Prefixed prerequisites

image

Side effect fixes:

This also fixes misaligned module trees as a side effect
image

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.

Copy link

vercel bot commented Oct 23, 2024

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 Oct 26, 2024 6:51am

Copy link

vercel bot commented Oct 23, 2024

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

Copy link

codecov bot commented Oct 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 54.76%. Comparing base (2d4743d) to head (bd7c8aa).
Report is 42 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3842      +/-   ##
==========================================
+ Coverage   53.87%   54.76%   +0.89%     
==========================================
  Files         274      275       +1     
  Lines        6032     6080      +48     
  Branches     1449     1461      +12     
==========================================
+ Hits         3250     3330      +80     
+ Misses       2782     2750      -32     

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

@zehata
Copy link
Author

zehata commented Oct 23, 2024

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

@zehata 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
Copy link
Author

zehata commented Oct 26, 2024

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

@ravern
Copy link
Member

ravern commented Oct 30, 2024

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
Copy link
Author

zehata commented Oct 30, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prerequisite tree right-to-left progression is confusing
2 participants