Misaligned module code in Prerequisite Tree

[Jiwei99]

Describe the bug

Some module code in the Pre-Requisite Tree is misaligned.

To Reproduce

Steps to reproduce the behavior:

Layout of FIN2704 is different from rest of the modules
https://nusmods.com/courses/FIN3716/financial-modelling

Module codes are not aligned properly, and the alignment is inconsistent
https://nusmods.com/courses/BSP3701A/strategic-management

Desktop (please complete the following information):

• OS: [MacOS]
• Browser [Chrome, Safari]

[zehata]

Looks like it's caused by FIN2704 being an available module, which wraps it in a button that is invisible

[zehata]

FIN2704 is an active mod that links to FIN2704/FIN2704X, so getModuleCondensed("FIN2704") is truthy but moduleActive for FIN2704 is falsey (name is not exact match). This causes the node to be a clickable button that appears invisible.

Fix is trivial but UX requires consideration, there are a few options:

1. Remove the button, keep the text. Preferred for consistency.
2. Keep the button, and make it visible
3. Keep the button, keep it invisible(??) Really strange UI

[Jiwei99]

@zehata What if "Course starting with" is replaced with an enumeration of all versions of the module using a "one of" branch? Think that would be more aligned with the design of the rest of the prerequisites trees (but not sure how feasible is this)

[zehata]

@Jiwei99 Something like this? I agree with you that this solution seems the cleanest, since it is more consistent with mods that don't use wildcard prerequisites.

However, I noticed that if the required grade is not D, the UI will look this: (not the prettiest)

Update: looks like doing it this way will also fix a previously unfound bug - if required grade is not D, the current UI will not show it because the prefix get overridden: https://github.com/nusmodifications/nusmods/blob/0f33a55fb2c446298016be8e8d499c030e099c4d/website/src/views/modules/ModuleTree.tsx#L56-L67

[zehata]

Alternatively:

[Jiwei99]

Alternatively:

Yeah I think this looks the cleanest, but should the grade requirement be shown? I don’t remember seeing it in the current prereq tree

[zehata]

The grade requirement is only shown when it is not the default 'D' grade.

[zehata]

I think it is an edge case in which prerequisites don't use the default 'D' passing grade AND use the "Courses starting with" pattern. So I think that is not of priority and could be fixed with a separate PR.

[Jiwei99]

Alternatively:

Ah, then I would think this looks the best. The other version with the prefix in the module button looks too chunky

[zehata]

It is possible to do this, but it's possible that situations like this will happen, which might appear messy to some people? (I think it's fine though). If it's possible, I hope someone from the core team can weigh in on this UI. (The tree below is just test data)

[zehata]

Also @ core team, an alternative is to update ANTLR to branch when encountering wildcard modules instead of appending % when scraping, which is a cleaner implementation but will also be more complex computationally on the server. I think practically speaking this frontend implementation is sufficient.

[ravern]

but will also be more complex computationally on the server.

I think this is fine -- we only run the scraper once every hour.

I can see this working by modifying this parse function to build up a trie of modules (opposed to a list) and then passing that to the parseString function, which can then expand modules it comes across. Alternatively we can add a new function expandWildcards after the parseString function call.

https://github.com/nusmodifications/nusmods/blob/0f33a55fb2c446298016be8e8d499c030e099c4d/scrapers/nus-v2/src/services/requisite-tree/index.ts#L27-L48

If we decide we want to expand wildcards, I'm slightly in favour of going the backend/scraper route, but would love to hear input from the other maintainers @nusmodifications/nusmods-developers.

---

But going back to earlier discussion, do we want to expand these "courses starting with" in the first place? I'm afraid that it might generate huge and unwieldy pre-req trees in some cases.

If anyone has some time, could we perhaps get a sense of what's the largest pre-req tree this could potentially generate (in terms of number of rows/lines)? Probably by writing a quick script that runs across all the modules in moduleList.json and expands everything.

---

I like this UI actually. Just have a question on the second (turquoise) branch where it says "one of" but only has one module. Is this just bad test data for the screenshot or is this a possible scenario?

[zehata]

@ravern Sorry about the long silence, I wasn't paying attention to this because exam season and also my github notifs aren't working for some reason...

[zwliew]

I don't really have an opinion on this for now, but I'll first merge a stopgap solution for the misalignment issue (#3883). We can work on further enhancements to the UI later on.