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

Editor: Remove global state placeholder actions #5438

Merged
merged 1 commit into from
May 24, 2016

Conversation

aduth
Copy link
Contributor

@aduth aduth commented May 18, 2016

This pull request seeks to effectively revert #3025 and #3202, removing placeholder actions from being called in the post editor, for the following reasons:

  • The intended direction for editing posts via global Redux state has changed dramatically since the original placeholders were added:
  • Dispatching placeholder actions has a negative effect on performance
    • Dispatching to the global store, even if there's no effective change in state, causes all mounted connect components to re-run their state selector logic
    • Subtle issues like getContent (a very slow function) being called multiple times
  • Inconsistently applied in newer refactorings (example)
  • Causes confusion to developers who may not understand the intent of the duplicated actions
  • Progress has stalled on fleshing out behavior of placeholder action creators

Testing instructions:

Ensure Mocha tests pass:

npm run test-client

Verify that there are no regressions in all affected files, which is effectively all editor behavior which has an effect on the values saved for a post.

Ensure no remaining references to state/ui/editor/post exist.

@aduth aduth added [Feature] Post/Page Editor The editor for editing posts and pages. [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. State labels May 18, 2016
@gwwar
Copy link
Contributor

gwwar commented May 20, 2016

This looks good so far @aduth. Ping me when you rebase and I can test again.

@aduth aduth force-pushed the remove/editor-redux-placeholder-actions branch from bd0a742 to f7400bd Compare May 23, 2016 11:58
@aduth
Copy link
Contributor Author

aduth commented May 23, 2016

Thanks @gwwar ! Rebased (single semi-colon caused it to conflict 😆 ).

Also added note to testing instructions about verifying what's been removed.

@gwwar
Copy link
Contributor

gwwar commented May 23, 2016

🚢

@gwwar gwwar added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels May 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Post/Page Editor The editor for editing posts and pages. State
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants