Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix prop-drilling in IDEView #2054

Closed
wants to merge 11 commits into from
Closed

Fix prop-drilling in IDEView #2054

wants to merge 11 commits into from

Conversation

lindapaiste
Copy link
Collaborator

This is a draft because the unit tests for <Preferences/> need to be updated.

Fixes #824
Fixes #2053
Continues work on #1458
Expands on some of my previous PRs.

Changes:

Props Drilling

I handled the following components:

  • Preferences
  • Sidebar
  • ShareModal

The IDEView no longer passes down any props to these components. They now take no props and access everything via react-redux hooks. I converted all three from class to function components.

Code Breakup

I moved all of the overlays from IDEView to a new component IDEOverlays, as suggested here. This new component is connected to the redux store which means that more prop drilling goes away, as the IDEView no longer needs to deal with the isVisible and close logic for the modals and overlays.

I have verified that this pull request:

  • has no linting errors (npm run lint)
  • has no test errors (npm run test)
  • is from a uniquely-named feature branch and is up to date with the develop branch.
  • is descriptively named and links to an issue number, i.e. Fixes #123

@lindapaiste
Combined Modal component for New File, New Folder, and Upload.
fc1e4b4
@lindapaiste
Move keydown handling out of IDEView
9597861
@lindapaiste
Merge branch 'refactor/redux-hooks' into refactor/modal
56ba238 
# Conflicts:
#	client/modules/IDE/pages/IDEView.jsx
@lindapaiste
Move overlays out of IDEView, connect sub-components instead of passi…
c1af181 
…ng props.
@lindapaiste
Merge branch 'develop' into refactor/props-drilling
a534ba2
@lindapaiste
Combined Modal component for New File, New Folder, and Upload.
027efbd
@lindapaiste
Move keydown handling out of IDEView
dd41459
@lindapaiste
Move overlays out of IDEView, connect sub-components instead of passi…
48e6b3d 
…ng props.
@lindapaiste
Merge remote-tracking branch 'origin/refactor/props-drilling' into re…
4bbb3aa 
…factor/props-drilling

# Conflicts:
#	client/modules/IDE/components/Editor.jsx
#	client/modules/IDE/components/Feedback.jsx
@lindapaiste
Refactor tests to render inside react-redux context.
72812e2
@lindapaiste lindapaiste changed the base branch from develop to release March 15, 2023 05:15
@lindapaiste lindapaiste changed the base branch from release to develop March 15, 2023 05:15
@lindapaiste lindapaiste mentioned this pull request Mar 26, 2023
Closed
4 tasks
@lindapaiste
Copy link
Collaborator Author

Just a quick check-in: I did end up refactoring the tests to work with new propsless/connected design.

There are still two tests which fail with regards to handling non-numeric inputs on the text size input. I changed this from an input with type “text” to type “number” which means that there is some automatic validation by the DOM. So I don’t think there is any actual problem, as you cannot type letters into the input. But it does cause the tests to fail 🤷🏻‍♀️

@raclim
Copy link
Collaborator

raclim commented Apr 19, 2023

Thanks for your update on this! Hmm, that makes sense to me! I'll also think on this on my end and will update you with any thoughts!

This was referenced Jul 26, 2023
Merged
Merged
Merged
Merged
@lindapaiste
Copy link
Collaborator Author

I'm going to close this as I have re-submitted most of the changes in individual PRs.

Reminder to myself -- I still need to revisit changing the font size input to type="number", which is the part that broke the tests. And also implement these clean ups of the JS.

 const validateFontSize = (number) => {
    if (Number.isNaN(number)) return 16;
    return clamp(number, 8, 36);
  };

  // The current contents of the input, which may be invalid. String or number.
  const [fontSizeString, setFontSizeString] = useState(fontSize);
  const fontSizeNumber = parseInt(fontSizeString, 10);

  // Immediately submit any valid fontSize input
  useEffect(() => {
    if (validateFontSize(fontSizeNumber) === fontSizeNumber) {
      dispatch(setFontSize(fontSizeNumber));
    }
  }, [fontSizeNumber]);

  // Handler for increase and decrease.
  const updateFontSize = (value) => {
    const valid = validateFontSize(value);
    setFontSizeString(valid);
    dispatch(setFontSize(valid));
  };

  const onFontInputSubmit = (event) => {
    event.preventDefault();
    const valid = validateFontSize(fontSizeNumber);
    dispatch(setFontSize(valid));
  };

  const fontSizeInputRef = useRef();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Prop Drilling vs. Redux Increase font size button can concatenate 20 => 202 instead of adding
2 participants
@lindapaiste @raclim