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.

Sign up for GitHub

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

Jump to bottom

Skyjumping Beavers && Post-editor add redux actions #2 #3202

Merged
merged 22 commits into from
Feb 17, 2016
Merged

Skyjumping Beavers && Post-editor add redux actions #2 #3202

merged 22 commits into from
Feb 17, 2016

Conversation

artpi
Copy link
Contributor

@artpi artpi commented Feb 9, 2016

Post-editor reduxification: add redux actions #2

Continuing work from #3025
Progress trakcing of whole effort: #2993

Parachuting Beavers (Clickbait)

I am now putting clickbait in my PRs to facilitate reviews

Did you know that after second world war Idaho Fish and Game commitee was parachuting beavers into remote areas? They were pretty badass!

Luckily for Idaho Fish and Game, there was a surplus of parachutes after the war. Boxes were hand-made out of willow wood and tied to the parachutes and, in 1948, 76 beavers were air-dropped into the area.

More info here

@artpi artpi added Framework [Feature] Post/Page Editor The editor for editing posts and pages. [Status] In Progress labels Feb 9, 2016
@artpi artpi added this to the Calypso Core: Offline 3 milestone Feb 10, 2016
@artpi artpi 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 Feb 10, 2016
@artpi artpi changed the title Post-editor reduxification: add redux actions #2 Skyjumping Beavers && Post-editor add redux actions #2 Feb 15, 2016
rralian
rralian reviewed Feb 15, 2016
View reviewed changes
client/post-editor/editor-ground-control/index.jsx
postActions.edit( { date: date ? date.format() : null } );
this.props.setDate( date );
Copy link
Contributor

Choose a reason for hiding this comment

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

I just want to make sure this was a conscious decision to move this logic (setting to date.format() or null) into the setDate reducer. Maybe we should add a note into the placeholder reducer about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is moved to action, not the reducer, but yes, it's a conscious decision, and it's a good idea to put some information there!
I generally am trying to sneak logic out of the view, the view should not be concerned about the format that API takes data in

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 0f662ce

@artpi artpi force-pushed the update/editor-duplicating-actions branch from 69904ed to 0f662ce Compare February 15, 2016 23:17
rralian
rralian reviewed Feb 15, 2016
View reviewed changes
client/post-editor/editor-featured-image/preview-container.jsx

export default React.createClass( {
const EditorFeaturedImagePreviewContainer = React.createClass( {
displayName: 'EditorFeaturedImagePreviewContainer',

propTypes: {
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we add setFeaturedImage as a propType?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, and set a default prop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in 2f78c97

@rralian
Copy link
Contributor

rralian commented Feb 15, 2016

I ran into this error when switching to html view in a draft:
js error1

@artpi
Copy link
Contributor Author

artpi commented Feb 15, 2016

I ran into this error when switching to html view in a draft:

I just rebased the whole thing and it seems some stuff moved around in the tree. Debugging!

@rralian
Copy link
Contributor

rralian commented Feb 15, 2016

I found one error and had a couple of questions, but overall this feels really solid to me.

@artpi
Copy link
Contributor Author

artpi commented Feb 16, 2016

Error fixed in 962a4bd

@rralian
Copy link
Contributor

rralian commented Feb 16, 2016

This looks great! :shipit:

@rralian rralian 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 Feb 16, 2016
artpi added a commit that referenced this pull request Feb 17, 2016
@artpi
Merge pull request #3202 from Automattic/update/editor-duplicating-ac…
d32c307 
…tions

Skyjumping Beavers && Post-editor add redux actions #2
@artpi artpi merged commit d32c307 into master Feb 17, 2016
@artpi
Copy link
Contributor Author

artpi commented Feb 17, 2016

Forgot to squash. Sorry :/

@gwwar gwwar mentioned this pull request Feb 19, 2016
Merged
@artpi artpi mentioned this pull request Feb 24, 2016
Closed
@aduth aduth mentioned this pull request Mar 11, 2016
Merged
@apeatling apeatling deleted the update/editor-duplicating-actions branch March 17, 2016 18:15
@aduth aduth mentioned this pull request May 18, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@artpi artpi

Labels
[Feature] Post/Page Editor The editor for editing posts and pages. Framework
Projects
None yet
Milestone
Calypso Core: Offline 5
Development

Successfully merging this pull request may close these issues.
3 participants
@artpi @rralian @scruffian