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.

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

Remove componentWillReceiveProps from ColorPicker #11772

Merged
merged 41 commits into from
May 8, 2019

Conversation

oandregal
Copy link
Member

@oandregal oandregal commented Nov 12, 2018

We want to get rid of componentWillReceiveProps method #11360

Some context

This is the ColorPicker tree:

ColorPicker

  • Saturation
  • Hue
  • Inputs
    • Input (for HEX)
    • Input (for RGB - actually, three inputs, one for each band)
    • Input (for HSL - actually, three inputs, one for each band)

The ColorPicker acts as a coordinator of the different color views (Saturation, Inputs) and keeps them in sync - when one changes the color, the others are updated. It uses some utils to work with colors (such as converting #fff to #ffffff and so on).

Besides those updates, the Input leaf components were updated when some internal state changed. They used this internal state as a cache. They didn't notify the ColorPicker of changes on every keystroke, only for certain kind of events (when the users hit the ENTER key, on blur, etc).

The problem

The Input leaf component was updated both on state and prop changes. It took advantage of componentWillReceiveProps only executing when props changed to differentiate state and props changes. This is considered an anti-pattern in React and the reason why they deprecated componentWillReceiveProps in favor of getDerivedStateFromProps.

Unlike componentWillReceiveProps, getDerivedStateFromProps is called when either props or state changes, so we no longer can distinguish why the method is called (is it a prop change or a state change?) within the same component. To deal with this use case React recommends centralizing all data in one place, either in a component state (they call this uncontrolled component) or props (they call this controlled component).

How this PR fixes it

We wanted the Input leaf components to react to changes in other views (Saturation, for example) by updating their props. To achieve this I needed to do two things:

  • Converting the Input components into controlled components. Lift up the cache (internal state) to the ColorPicker by distinguishing when a change needs to be commited (spread to the other views) or when a change is a draft (should only affect the Inputs view).
  • Centralizing all the logic to decide when to commit changes to the colors. It was previously spread through several components, for example, the Inputs component checked if the Hex code was valid and the ColorPicker checked whether the RGB/HSL was valid to back out from notifying all the views of the color change.

Testing

  • Test that changes in one view update the others.
  • Test color changes only the keyboard, to make sure focus is preserved.

@oandregal oandregal self-assigned this Nov 12, 2018
@oandregal oandregal added the Framework Issues related to broader framework topics, especially as it relates to javascript label Nov 12, 2018
@mtias mtias added this to the 4.4 milestone Nov 12, 2018
@mtias mtias added the [Type] Code Quality Issues or PRs that relate to code quality label Nov 12, 2018
Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Things look good in my tests 👍

@oandregal oandregal force-pushed the update/react-colorpicker branch from 750f4ce to cf38534 Compare November 13, 2018 12:57
@oandregal oandregal added the [Status] In Progress Tracking issues with work in progress label Nov 13, 2018
@oandregal
Copy link
Member Author

Ugh, this has taken more time than I expected. The CodePicker component tree suffered from a couple of smells that made it difficult to work with. I've updated the description to better explain the changes here so you may want to read that first.

I still need to update the tests I've added in this PR to the new changes, but wanted to give you as much time as possible for review.

@gziolo gziolo requested a review from a team November 14, 2018 11:28
@oandregal oandregal removed the [Status] In Progress Tracking issues with work in progress label Nov 14, 2018
@youknowriad youknowriad modified the milestones: 4.4, 4.5 Nov 15, 2018
@oandregal oandregal force-pushed the update/react-colorpicker branch from ceb7860 to be0cc4f Compare November 15, 2018 12:53
@oandregal
Copy link
Member Author

I've fixed all the concerns I had about this PR and this is ready for review/merge.

@jorgefilipecosta jorgefilipecosta dismissed their stale review November 15, 2018 15:29

Additional changes happened. Author requested a new review.

@mtias mtias modified the milestones: 4.5, 4.6, 4.7 Nov 19, 2018
@youknowriad youknowriad modified the milestones: 4.7, 4.8 Dec 4, 2018
@youknowriad youknowriad modified the milestones: 4.8, 4.9 Dec 19, 2018
so they don't get re-rendered when draft values change.
By splitting handleChange in smaller methods, we can target
which value to update without relying on the data properties.
This brings back the ability to add empty values.
This prevents leaving views with invalid or empty values.
Before this change, the input components were responsible to send
the value that changed and the untouched ones. After this change,
the input components only send the value that changed
and the index is responsible to merge this into the old color object.
@oandregal oandregal force-pushed the update/react-colorpicker branch from 91e8ae8 to 4c202f7 Compare March 15, 2019 11:49
@oandregal
Copy link
Member Author

@jorgefilipecosta that was a good catch, thanks! :)

@jorgefilipecosta @mcsf @ryelle It took me a few days to get back to this, sorry about that. The bug was caused by an extra check (isEmptyColor) for Saturation and Hue that I had inadvertently added in the refactor and that isn't present in master. I tested this on FF and Chrome (Ubuntu Linux):

Peek 2019-03-15 12-48

Peek 2019-03-15 12-49

@youknowriad youknowriad removed this from the 5.3 (Gutenberg) milestone Mar 18, 2019
@oandregal
Copy link
Member Author

@jorgefilipecosta this is ready for a review.

@oandregal oandregal modified the milestone: 5.6 (Gutenberg) Apr 23, 2019
Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my tests, I did not found any regression so I think this PR is ready. Thank you @nosolosw for all the iterations performed. This is a nice step in removing deprecated react methods.

As noted during the discussions, we may have an opportunity to simplify the way these components work based on the prototype code shared.

@oandregal oandregal merged commit cdfb97c into master May 8, 2019
@oandregal oandregal deleted the update/react-colorpicker branch May 8, 2019 10:09
@youknowriad youknowriad added this to the 5.7 (Gutenberg) milestone May 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants