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.
• [N/A] I have added tests for my changes, if applicable.
  • This is required for all bug fixes and new features.
• [N/A] I have updated the project documentation, if applicable.
  • This is required for new features.
• [N/A] 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.

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

[coveralls]

Pull Request Test Coverage Report for Build 2bca8f6f-d54f-47fc-a183-b940959a2751

Details

• 9 of 12 (75.0%) changed or added relevant lines in 4 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.05%) to 50.737%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
js/components/common/export.js.jsx	1	2	50.0%
js/components/graph/Container.js	3	4	75.0%
js/components/grid/grid.js.jsx	4	5	80.0%

Totals
Change from base Build cd31ef18-07d3-4d4e-981e-35b39bd29d7d:	-0.05%
Covered Lines:	1770
Relevant Lines:	3548

---

💛 - Coveralls

[david-yz-liu]

@akarki2005 all of the code changes look great.

Now that we've added this component to under the graph/grid components, please add some tests for the export modal functionality.

(It doesn't seem like there were any tests for the export modal functionality before, so this is a great time to add some!)

[david-yz-liu]

@akarki2005 the tests you added were good, but please also add some functional tests for the graph/grid components to check that clicking the export button also causes the modal to appear.

[akarki2005]

Hi @david-yz-liu, I've added tests for both the graph and grid pages to make verify the functionality of the export button. In doing this, I also had to move the DOM mounting code located at the bottom of grid.js.jsx into a main file for the grid page, similar to what is already done for the graph page. Without this, I was getting this error when running tests:

js/components/grid/__tests__/GridExport.test.js
  ● Test suite failed to run

    Target container is not a DOM element.

      196 |
      197 | const container = document.getElementById("grid-body")
    > 198 | const root = createRoot(container)
          |                        ^
      199 | root.render(<Grid />)

      at Object.<anonymous>.process.env.NODE_ENV.exports.createRoot (node_modules/react-dom/cjs/react-dom-client.development.js:24878:15)
      at Object.<anonymous> (js/components/grid/grid.js.jsx:198:24)
      at Object.require (js/components/grid/__tests__/GridExport.test.js:3:1)

Please let me know if any changes are required - thanks!

[david-yz-liu]

@akarki2005 the changes are all good, except when moving the "main" code of the grid component into a separate file, you also need to make changes to the actual HTML of the front-end in order to load this file and get the Grid component to display!

[david-yz-liu]

Nice work, @akarki2005!