-
Notifications
You must be signed in to change notification settings - Fork 2k
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
State: Remove computed attributes from Redux site object #4060
Conversation
} | ||
|
||
this.props.dispatch( action ); | ||
this.props.dispatch( signup( this.props ) ); |
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.
Was the previous if block a bad copy, or prep for future usage?
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.
Was the previous if block a bad copy, or prep for future usage?
It's my assumption, one which should be confirmed by the themes folks, that the other actions were never reached, due to the fact that selectedSite
was never being passed (current use-cases render the non-connected component). It's also not as simple as replacing with sitesList.getSelectedSite()
, since the sites-list
module is not server-render compatible.
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.
@seear ^
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 is there for future usage, but given that it doesn't work it is fine to rip it out for now.
👍 Changes look good. It would be good to get another +1 on the Themes changes before merging. I found an unrelated bug with removing connections. Let me know if you'd like an issue:
|
Would I be correct in assuming then that this only affects environments where Redux persistence is enabled? (May affect prioritization) |
I believe so, though in prod it still looks like I still need to refresh the page to see the connection removed in editor. (Navigating between the sharing section and editor doesn't update the view) |
d37e5b3
to
51227dd
Compare
Since assumes presence of computed attributes
Assumes existence of computed site attributes. Currently no flow which would result in `site` being provided to ThemeSheet component
51227dd
to
bcc5331
Compare
Looks good, and the themes stuff tests out fine 👍 |
Thanks for the reviews, all. |
State: Remove computed attributes from Redux site object
Fixes #2757
Related: #4057
This pull request seeks to resolve remaining issues preventing us from persisting the Redux site state. Because some areas of the application had already depended upon site objects containing decorated attributes, changes were required to remove any such dependencies before site state could be persisted.
Testing instructions:
Ensure Mocha tests pass by running
npm test
Verify that no regressions occur in affected components. Specifically:
/cc @gwwar , @ockham , @seear , @ehg