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 redux: passing IDs to action creators & Washing machine madness #3547

Closed
wants to merge 29 commits into from

Conversation

artpi
Copy link
Contributor

@artpi artpi commented Feb 24, 2016

What does it solve?

In PR #3202 I introduced atomic action creators to interact with currently edited post.
In PR #3183 @aduth has introduced editPost method which requires postId and siteId.

This PR provides post and siteId to the setTitle action creator as an example

[clickbait] Once you review, check this out:

Hans Rosling, Swedish statistitian proved that the most important achievement of the industrial revolution is a Washing Machine! I highly recommend his energetic and fascinating TED talk: https://www.ted.com/talks/hans_rosling_and_the_magic_washing_machine.

This is in progress, but I want to decide on the approach before I go through all of these actions.
CC @gwwar @aduth @rralian

@artpi artpi added [Status] In Progress [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 24, 2016
@@ -39,6 +42,8 @@ const EditorTitle = React.createClass( {

getDefaultProps() {
return {
siteId: 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the default siteId 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So it's not any valid siteId?
You are for null ? I always have trouble deciding... :/

@gwwar
Copy link
Contributor

gwwar commented Feb 25, 2016

👍 Looking good so far. Ping me again when you're ready for another review.

}

export function getCurrentEditedPostId( state ) {
return state.ui.editor.post.currentEditedPostId;
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably move this selector to be alongside state/ui/editor/post. Edit: hmm, looks like we're grouping all of these at the top level.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I assume this tree will be trimmed and shaped a lot.
I wanted to keep it at top level so we dont have to edit files that import this selector while we toss and turn tree structure.

@artpi
Copy link
Contributor Author

artpi commented Feb 28, 2016

@aduth: this PR should be around ready, but needs heavy testing. I will try to find time for it during the support rotation.

@aduth
Copy link
Contributor

aduth commented Feb 29, 2016

There are some failing tests here.

@@ -16,6 +16,7 @@ export const EDITOR_CONTACT_FORM_ADD_DEFAULT_FIELD = 'EDITOR_CONTACT_FORM_ADD_DE
export const EDITOR_CONTACT_FORM_CLEAR_FORM = 'EDITOR_CONTACT_FORM_CLEAR_FORM';
export const EDITOR_CONTACT_FORM_LOAD_FORM = 'EDITOR_CONTACT_FORM_LOAD_FORM';
export const EDITOR_CONTACT_FORM_REMOVE_FIELD = 'EDITOR_CONTACT_FORM_REMOVE_FIELD';
export const EDITOR_POST_CURRENT_ID = 'EDITOR_POST_CURRENT_ID';
Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably have some sort of verb in this action type name. Consider suffixing with _SET

@artpi artpi force-pushed the update/redux-editor-actions-id branch from 8792410 to 4fd5b04 Compare February 29, 2016 23:48
@artpi artpi force-pushed the update/redux-editor-actions-id branch from 4fd5b04 to 3cf1fe9 Compare March 10, 2016 13:38
@artpi
Copy link
Contributor Author

artpi commented Mar 18, 2016

We will hold off this PR for now.
getSelectedSiteId selector is going to #4149

@artpi artpi closed this Mar 18, 2016
@scruffian scruffian removed [Status] In Progress [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Mar 18, 2016
@lancewillett lancewillett deleted the update/redux-editor-actions-id branch April 7, 2016 18:20
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.

4 participants