Components: Turn a few into Component classes #19102

ockham · 2017-10-24T12:02:40Z

Modeled after #19085. I grepped for this.getInitialState() and did the following (same as #19085 (comment)):

Temporarily replace the this.getInitialState() call with a dummy (I used a string, 'intialState' 😄 )
Replace createReactClass with React.createClass so the codemod would pick it up
Run the codemod
Introduce the intialState static, use it to init state, and replace the dummy 'initialState' string with it.

To test: Verify that the affected components still work as before.

matticbot · 2017-10-24T12:02:41Z

Test live: https://calypso.live/?branch=update/react-component-classes

sirreal · 2017-10-24T12:27:57Z

client/blocks/term-tree-selector/terms.jsx

+	// initialState is also used to reset state when a the taxonomy prop changes
+	static initialState = {
+		searchTerm: '',
+		requestedPages: [ 1 ],


The operations on this.state.requestedPages seem to be safe, but with this change I'd advocate using Object.freeze on any array/object in Component.defaultProps or Component.initialState.

sirreal · 2017-10-24T12:36:20Z

client/blocks/term-tree-selector/terms.jsx

+		analyticsPrefix: 'Category Selector',
+		searchThreshold: 8,
+		loading: true,
+		terms: [],


Want to Object.freeze this too?

Oops, oversight of mine. ccd4ee7

ockham · 2017-10-24T12:36:55Z

Discovered something pre-existing that didn't seem right: 2811098

sirreal · 2017-10-26T07:41:37Z

client/blocks/term-tree-selector/terms.jsx

@@ -3,10 +3,8 @@
 *
 * @format


Want to pull @format out of external deps?

I've looked over this file and smoke tested via the post editor. Looks good 👍

(a review in parts)

sirreal · 2017-10-26T07:57:20Z

client/components/data/followers-data/index.jsx

@@ -3,9 +3,8 @@
 *


I smoke tested followers-data and email-followers-data and there seem to be no regressions 👍

Interesting, first time I've seen the DataComponent wrapper in Calypso 🤔

sirreal · 2017-10-26T07:57:27Z

client/components/data/email-followers-data/index.jsx

@@ -3,9 +3,8 @@
 *


I smoke tested followers-data and email-followers-data and there seem to be no regressions 👍

Interesting, first time I've seen the DataComponent wrapper in Calypso 🤔

Also fixes an incorrect component name in README

sirreal · 2017-10-26T08:19:34Z

client/components/data/viewers-data/index.jsx

@@ -3,9 +3,8 @@
 *


👍 Smoke tested and learned about viewers (only available on private, non-Jetpack sites).

sirreal · 2017-10-26T08:20:56Z

I pushed a commit which names exported default classes. The names were lost since displayName removal, but can now be read via class names.

sirreal · 2017-10-26T09:20:50Z

client/components/site-selector/index.jsx

 		highlightedIndex: -1,
 		showSearch: false,
 		isKeyboardEngaged: false,
 	};

+	state = this.constructor.initialState;
+
 	reset() {


I was trying to trigger this to confirm correct behavior, but it appears to be unused. Can you find whether this method is referenced anywhere? We might be able to simplify this further and not need to make the initialState changes.

Looks like this was introduced in #5808 by @marekhrabe (8b08a8f).

Can you say whether it's safe to remove this method which doesn't appear to be called?

👍 I'd like to rip it out if possible, but this file looks good either way.

Reset is not in use right now. It was part of work for #8739 that wasn't implemented. I think it's safe to remove now and simplify the initialState declaration. It can be easily reintroduced once we are sure we need it.

sirreal · 2017-10-26T09:22:44Z

client/components/data/viewers-data/README.md

@@ -9,10 +9,10 @@ A component that fetches a private wpcom site's viewers and passes them to its c

 ## Usage

-A component wrapped with `<FollowersData />` will receive the following props:


Just fixing wrong component reference.

sirreal · 2017-10-26T09:35:27Z

client/me/purchases/cancel-privacy-protection/index.jsx

@@ -3,15 +3,12 @@
 *


Smoke tested, works well 👍

sirreal · 2017-10-26T09:57:00Z

client/post-editor/editor-featured-image/preview.jsx

@@ -3,10 +3,8 @@
 *


👍 smoke tested

sirreal

💪 Everything seems to work well, nicely done!

It would be nice to completely remove the reset and the initialState it requires if we can determine that's safe.

marekhrabe · 2017-10-26T10:08:16Z

Great work on this PR! 💖

ockham · 2017-10-26T19:00:29Z

Thanks a lot for reviewing @sirreal and for chiming in @marekhrabe!

It would be nice to completely remove the reset and the initialState it requires if we can determine that's safe.

907d961

@sirreal

Most of these weren't migrated by the `create-class` codemod before because they contained a reference to `this.getInitialState()` in some method (to reset the component to its initial state after some user interaction). This can be resolved by defining `static initialState = {...}; state = this.constructor.initialState;`, and replacing the reference to `this.getInitialState()` by `this.constructor.initialState`. Since `SiteSelector` wasn't actually using the `reset()` method which referenced `this.getInitialState()`, that method was removed altogether (props @sirreal!). Finally, this PR fixes an incorrect component name in `<ViewersData />`'s README, as well as the affected components' `@format` comment headers.

ockham added 6 commits October 24, 2017 14:00

Editor: Turn featured image preview into Component class

c5f3e7f

TermTreeSelector: Turn Terms into Component class

f68b400

EmailFollowersData: Turn into Component class

9730f69

FollowersData: Turn into Component class

8b0199f

ViewersData: Turn into Component class

c175ca4

CancelPrivacyProtection: Turn into Component class

3671e3a

ockham added Components [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Oct 24, 2017

ockham self-assigned this Oct 24, 2017

ockham requested review from sirreal and samouri October 24, 2017 12:02

sirreal reviewed Oct 24, 2017

View reviewed changes

ockham added 3 commits October 24, 2017 14:32

Actually make initialState static

91667bd

Freeze

fbbb1bf

SiteSelector: Fix this.getInitialState() call

2811098

sirreal reviewed Oct 24, 2017

View reviewed changes

Moar freeze

ccd4ee7

ockham requested review from jsnajdr, jsnmoon and spen October 24, 2017 13:36

sirreal added the [Type] Janitorial label Oct 26, 2017

sirreal reviewed Oct 26, 2017

View reviewed changes

Name exported default component classes

4aab585

Also fixes an incorrect component name in README

sirreal reviewed Oct 26, 2017

View reviewed changes

Remove body braces from fat-arrow function

688f398

sirreal reviewed Oct 26, 2017

View reviewed changes

client/me/purchases/cancel-privacy-protection/index.jsx Outdated

@@ -3,15 +3,12 @@

*

Copy link

Member

sirreal Oct 26, 2017

Choose a reason for hiding this comment

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

Smoke tested, works well 👍

sirreal reviewed Oct 26, 2017

View reviewed changes

client/post-editor/editor-featured-image/preview.jsx Outdated

@@ -3,10 +3,8 @@

*

Copy link

Member

sirreal Oct 26, 2017

Choose a reason for hiding this comment

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

👍 smoke tested

sirreal approved these changes Oct 26, 2017

View reviewed changes

ockham added 2 commits October 26, 2017 20:53

SiteSelector: Remove unused reset() method, simplify initial state

907d961

Components: Fix a bunch of @Format headers

3563cee

ockham merged commit e84d803 into master Oct 26, 2017

ockham deleted the update/react-component-classes branch October 26, 2017 19:35

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Oct 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Components: Turn a few into Component classes #19102

Components: Turn a few into Component classes #19102

ockham commented Oct 24, 2017 •

edited

Loading

matticbot commented Oct 24, 2017

sirreal Oct 24, 2017

ockham Oct 24, 2017

sirreal Oct 24, 2017

ockham Oct 24, 2017 •

edited

Loading

ockham commented Oct 24, 2017

sirreal Oct 26, 2017

sirreal Oct 26, 2017

sirreal Oct 26, 2017

sirreal Oct 26, 2017

sirreal commented Oct 26, 2017

sirreal Oct 26, 2017 •

edited

Loading

marekhrabe Oct 26, 2017

sirreal Oct 26, 2017

sirreal Oct 26, 2017

sirreal Oct 26, 2017

sirreal left a comment

marekhrabe commented Oct 26, 2017

ockham commented Oct 26, 2017

		@@ -9,10 +9,10 @@ A component that fetches a private wpcom site's viewers and passes them to its c

		## Usage

		A component wrapped with `<FollowersData />` will receive the following props:

Components: Turn a few into Component classes #19102

Components: Turn a few into Component classes #19102

Conversation

ockham commented Oct 24, 2017 • edited Loading

matticbot commented Oct 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ockham Oct 24, 2017 • edited Loading

Choose a reason for hiding this comment

ockham commented Oct 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirreal commented Oct 26, 2017

sirreal Oct 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirreal left a comment

Choose a reason for hiding this comment

marekhrabe commented Oct 26, 2017

ockham commented Oct 26, 2017

ockham commented Oct 24, 2017 •

edited

Loading

ockham Oct 24, 2017 •

edited

Loading

sirreal Oct 26, 2017 •

edited

Loading