Make NavBar React (Part 2)

[akarki2005]

Proposed Changes

This change builds off of the previous PR that converted the navigation bar into a React component. With this PR, the codebase is refactored so that the display of the export modal relies entirely on React state, rather than direct DOM manipulation. To achieve this, state information about whether the modal is currently open was lifted up into root-level components for both the graph and grid pages (Container.js and Grid.js.jsx respectively). Additionally, to help facilitate this change, the navigation bar was shifted inside these root-level components, so that clicking the export button on the NavBar can open up the export modal (update state) without the use of vanilla DOM manipulation.

Type of Change

(Write an X or a brief description next to the type or types that best describe your changes.)

Type	Applies?
🚨 Breaking change (fix or feature that would cause existing functionality to change)
✨ New feature (non-breaking change that adds functionality)
🐛 Bug fix (non-breaking change that fixes an issue)
🎨 User interface change (change to user interface; provide screenshots)
♻️ Refactoring (internal change to codebase, without changing functionality)	X
🚦 Test update (change that only adds or modifies tests)
📦 Dependency update (change that updates a dependency)
🔧 Internal (change that only affects developers or continuous integration)

Checklist

(Complete each of the following items for your pull request. Indicate that you have completed an item by changing the [ ] into a [x] in the raw text, or by clicking on the checkbox in the rendered description on GitHub.)

Before opening your pull request:

• I have performed a self-review of my changes.
  • Check that all changed files included in this pull request are intentional changes.
  • Check that all changes are relevant to the purpose of this pull request, as described above.
• I have added tests for my changes, if applicable.
  • This is required for all bug fixes and new features.
• I have updated the project documentation, if applicable.
  • This is required for new features.
• If this is my first contribution, I have added myself to the list of contributors.
• I have updated the project Changelog (this is required for all changes).

After opening your pull request:

• I have verified that the CircleCI checks have passed.
• I have requested a review from a project maintainer.

Questions and Comments

Please see comment below!

[akarki2005]

Hi @david-yz-liu, I've done some work here to move towards the goal of having the export modal be controlled using React state rather than direct DOM manipulation, however I'm running into some failures with the pre-existing tests for the graph dropdown, and I'm unsure how to proceed. Right now, the tests in js/components/graph/__tests__/GraphDropdown.test.js are failing with this error message:

TestingLibraryElementError: Found multiple elements with the text: Graph

    Here are the matching elements:

    Ignored nodes: comments, script, style
    <a
      href="/graph"
    >
      Graph
    </a>

    Ignored nodes: comments, script, style
    <a
      href="/graph"
    >
      Graph
    </a>

    (If this is intentional, then use the `*AllBy*` variant of the query (like `queryAllByText`, `getAllByText`, or `findAllByText`)).

After some digging, I found that the duplicate "Graph" elements are coming from two sources: one is part of the actual React NavBar, and the other is injected into the test DOM via js/setupTestsAfterEnv.js.

This didn’t cause issues in my first NavBar PR because the NavBar was rendered at the root level and wasn't included within the Container component (js/components/graph/Container.js), which is what these failing tests render. However, now that I've moved the NavBar inside the top-level container for the graph page, the duplication is apparent.

I'm looking for guidance on the best way to go about resolving this. Thanks in advance!

[david-yz-liu]

Hi @akarki2005, thanks for your detailed comment! Honestly with the change that you're working on, I don't think pre-rendering the nav bar in js/setupTestsAfterEnv.js is necessary anymore. Please remove that and then see how that affects the tests. For some of the existing graph/timetable tests that check the export functionality, you might need to change the root component to be the "parent" component rather than the existing "container" component.