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

SDK: Try wp.data with calypso state tree "rewrite" #26930

Closed
wants to merge 6 commits into from

Conversation

ockham
Copy link
Contributor

@ockham ockham commented Aug 27, 2018

WIP. Alternative to #26838.

Includes #26944, so review & merge that PR before rebasing this one.

  • Use existing Calypso actions, selectors, and reducers.
  • Also used via withSelect/withDispatch
  • Use @wordpress/data's RegistryProvider
  • Selectors need to be modified to use local rather than global state (i.e. change state.subtree.something to state.something)
  • All uses of modified selectors need to be changed from connect to withSelect

To Test:

  1. Start calypso in local dev mode as usual.
  2. Go to http://calypso.localhost:3000/me/account and change the "Admin Color Scheme"
  3. Behavior should be the same, but it's using withSelect now.

@matticbot
Copy link
Contributor

@ockham ockham added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Aug 28, 2018
errorLogger.saveDiagnosticReducer( () => ( { tests: getSavedVariations() } ) );
analytics.on( 'record-event', ( eventName, eventProperties ) =>
errorLogger.saveExtraData( { lastTracksEvent: eventProperties } )
if ( config.isEnabled( 'catch-js-errors' ) && ! document.getElementById( 'primary' ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This code seems extraneous to this PR's purpose. If it's a separate improvement, would it make sense to put it in a standalone PR?

Copy link
Contributor Author

@ockham ockham Aug 29, 2018

Choose a reason for hiding this comment

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

It's in #26944 for separate review, but I'm leaving it included on this branch since it wouldn't work otherwise. (tl;dr this extraneous -- and obsolete -- empty Layout would leave wpRegistry undefined upon first render of RegistryProvider, and that breaks things quite badly).

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. If you want them reviewed separately, you may want to switch the base of this PR to that branch then.

@simison
Copy link
Member

simison commented Apr 3, 2019

I suppose this can be closed?

@sirreal
Copy link
Member

sirreal commented Nov 19, 2020

I'll close this, feel free to reopen if you'd like.

@sirreal sirreal closed this Nov 19, 2020
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 19, 2020
@simison simison deleted the try/wp-data-calypso-rewrite branch November 19, 2020 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants