-
Notifications
You must be signed in to change notification settings - Fork 70
Refactor GraphDropdown component from being a child of Graph to being a child of NavBar
#1637
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
base: master
Are you sure you want to change the base?
Refactor GraphDropdown component from being a child of Graph to being a child of NavBar
#1637
Conversation
| import { CourseModal } from "../common/react_modal.js.jsx" | ||
| import { ExportModal } from "../common/export.js.jsx" | ||
| import { getProgram } from "../common/utils.js" | ||
| import { getPost } from "../common/utils.js" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is unrelated to this PR
| setDropdownTimeouts(dropdownTimeouts.concat(timeout)) | ||
| } | ||
|
|
||
| React.useEffect(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The indentation at this line is incorrect
| } | ||
|
|
||
| React.useEffect(() => { | ||
| const navGraph = document.querySelector("#nav-graph") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part of your task is to remove DOM manipulation. This includes accessing DOM elements direction and manually adding callbacks (addEventListener, below). Instead, these callbacks should be managed by the React components themselves. Make sure you understand the existing behaviour (on master) so that you can replicate it with your changes.
| this.state.infoboxTimeouts.forEach(timeout => clearTimeout(timeout)) | ||
| this.setState({ infoboxTimeouts: [] }) | ||
| break | ||
| case TIMEOUT_NAMES_ENUM.DROPDOWN: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's now possible that TIMEOUT_NAMES_ENUM.DROPDOWN is no longer used anywhere. Check this, and if so please delete the DROPDOWN value from the enum.
| data-testid={"test-graph-dropdown"} | ||
| style={{ left: graphTabLeft }} | ||
| style={{ left: graphTabLeft, top: "50px", zIndex: 1000 | ||
| }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put the }} on the same line as the {{.
Also, in this file there is still direct DOM manipulation, which you should remove by handling the logic in the parent NavBar component.
Proposed Changes
GraphDropdowncomponent from being a child of Graph to being a child of NavBarType of Change
(Write an
Xor a brief description next to the type or types that best describe your changes.)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:
After opening your pull request: