Hide Columns

[KevinWu098]

Summary

Columns can now be shown/hidden through a dropdown menu in the Course Render Pane

Columns

• Columns can now be hidden by deselecting them in a new Dropdown Menu housed in CoursePaneButtonRow
  • Active columns are stored and emitted in RightPaneStore.ts

Removing ConditionalWrapper

• ConditionalWrapper.tsx has been removed in favor of plain conditional rendering logic.

• Just use conditional rendering logic instead of a goofy component. JSX is already a pain to read, there's no need to obscure component logic even more.

Removing colorAndDelete

• The property colorAndDelete, used in SectionTableBody (and related files) has been removed in favor of more reasonably calculating the proper value for addedCourse in sectionTableBody.tsx

Previously, sectionTableBody.tsx would always pass colorAndDelete as false, but then immediately recalculate it with the addedCourse state when it rendered:

const SectionTableBody = withStyles(styles)((props: SectionTableBodyProps) => {
    const { classes, section, courseDetails, term, 
      colorAndDelete /*ALWAYS false in courseRenderPane*/, 
      allowHighlight, scheduleNames } = props;

    const [addedCourse, setAddedCourse] = useState(colorAndDelete);

{...}

    // Immediately resets addedCourse
    useEffect(() => {
        const updateHighlight = () => {
            const doAdd = AppStore.getAddedSectionCodes().has(`${section.sectionCode} ${term}`);
            setAddedCourse(doAdd);
        };

The new callback doesn't immediately setAddedCourse, so instead we should just set the default useState to:

const [addedCourse, setAddedCourse] = useState(
        AppStore.getAddedSectionCodes().has(`${section.sectionCode} ${term}`)
    );

Doing so eliminates the need for the colorAndDelete prop!

Index.ts changes

• The way the app is rendered has been changed in index.tsx to an async function

Refactor scheduleConflict

• Refactored and added new helper functions into calendarizeHelpers.ts to assist in calculating scheduleConflicts
• Utilize useMemo and useCallback to more correctly manage state

Test Plan

• Columns should correctly be derendered when they're deselected in the Menu
• Make sure it functions well on all devices

Issues

Zotcourse parity a la #440

[MinhxNguyen7]

Sorry for taking so long to review. I missed the notification.

Great feature and works well. There are only some code quality issues.

My main issue is that there's so much separate state management and duplication going on. It's difficult to parse logic that should be relatively straightforward. Instead of having slightly different and separate states, try to put it all in one place, have named types, and translate it at the very end when you use it if really necessary.

Also, remember to merge. (I know it's my fault that there's a commit on main)

I'll review it

[ap0nia]

I think the changes look fine. I'll wait for the preview deployment to finish before merging.

[EricPedley]

Bumping this PR, seems like all that's left is to fix some merge conflicts

[ap0nia]

wait, stop

[ap0nia]

What are you doing, don't do this

  useEffect(() => {
        const updateHighlight = useCallback(() => {
          setAddedCourse(AppStore.getAddedSectionCodes().has(`${section.sectionCode} ${term}`));
        }, [setAddedCourse, AppStore.getAddedSectionCodes]);

        const checkAndDisplayScheduleConflict = () => {
            if (AppStore.getCourseEventsInCalendar().length < 1) {
                setScheduleConflict(false);
                return;
            }

[ap0nia]

Don't invoke hooks inside of other hooks

[KevinWu098]

💀 I did a whoopsie

[ap0nia]

Ok, fine, give me a bit to fix it

[KevinWu098]

Sorry 'bout all that nonsense, hopefully this latest commit should resolve the issues, but I'll test on the staging instance // read over the code again right now!

[ap0nia]

Ok no like. The entire strategy for figuring out conflicts is just wrong. There shouldn't even be a useEffect + useState. The entire logic should be a single useMemo

[ap0nia]

Ok, code is fixed. I have to go so I can't immediately verify the behavior live. Somebody else please review 🙏

TLDR:
Please don't write bad code. And if you don't know React, try consulting somebody that does before approving React code.

[KevinWu098]

Deployed staging instance to https://staging-624.antalmanac.com

I'll eventually move this into the original PR body, but for now, this is why I removed colorAndDelete:

Previously, sectionTableBody.tsx would always pass colorAndDelete as false, but then immediately recalculate it with the addedCourse state when it rendered:

const SectionTableBody = withStyles(styles)((props: SectionTableBodyProps) => {
    const { classes, section, courseDetails, term, 
      colorAndDelete /*ALWAYS false in courseRenderPane*/, 
      allowHighlight, scheduleNames } = props;

    const [addedCourse, setAddedCourse] = useState(colorAndDelete);

{...}

    // Immediately resets addedCourse
    useEffect(() => {
        const updateHighlight = () => {
            const doAdd = AppStore.getAddedSectionCodes().has(`${section.sectionCode} ${term}`);
            setAddedCourse(doAdd);
        };

The new callback doesn't immediately setAddedCourse, so instead we should just set the default useState to:

const [addedCourse, setAddedCourse] = useState(
        AppStore.getAddedSectionCodes().has(`${section.sectionCode} ${term}`)
    );

Doing so eliminates the need for the colorAndDelete prop!

[ap0nia]

Ok good enough, if it breaks prod I'll fix it later. 🚀