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

Themes: Add theme-details Redux subtree and data fetcher component #3011

Merged
merged 5 commits into from
Feb 12, 2016

Conversation

ockham
Copy link
Contributor

@ockham ockham commented Feb 2, 2016

Use the REST API v1.1's /theme endpoint for now.

Relevant to #2875

@ockham ockham added [Feature Group] Appearance & Themes Features related to the appearance of sites. [Status] In Progress labels Feb 2, 2016
@ockham ockham self-assigned this Feb 2, 2016
@ockham ockham added this to the Themes: Showcase M4-ThemeSheets milestone Feb 2, 2016
@ehg ehg mentioned this pull request Feb 2, 2016
6 tasks
@ockham ockham force-pushed the add/theme-details-redux-store branch 3 times, most recently from 202a49d to 61e2d79 Compare February 4, 2016 20:06
@ockham ockham changed the title Themes: Add theme-details Redux subtree Themes: Add theme-details Redux subtree and data fetcher component Feb 4, 2016
@ockham ockham mentioned this pull request Feb 4, 2016
3 tasks
@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 Feb 4, 2016
@ockham ockham force-pushed the add/theme-details-redux-store branch from 4449cd9 to 6b97af8 Compare February 4, 2016 21:16
@@ -65,5 +64,5 @@ export default connect(
currentTheme: getCurrentTheme( state, props.site.ID )
}
),
bindActionCreators.bind( null, { fetchCurrentTheme } )
{ fetchCurrentTheme }
Copy link
Member

Choose a reason for hiding this comment

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

Why remove the binding? Doesn't this break fetching in refresh?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/rackt/react-redux/blob/master/docs/api.md#connectmapstatetoprops-mapdispatchtoprops-mergeprops-options:

  • [mapDispatchToProps(dispatch, [ownProps]): dispatchProps] (Object or Function): If an object is passed, each function inside it will be assumed to be a Redux action creator. An object with the same function names, but bound to a Redux store, will be merged into the component’s props.

So bindActionCreators is apparently redundant here and can be safely dropped, since connect() does the binding for us.

Copy link
Member

Choose a reason for hiding this comment

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

Aha! I didn't remember that at all. :)

@seear
Copy link
Contributor

seear commented Feb 9, 2016

A confusing thing at the moment is that this new theme-details section of state tree has less information than the existing themes section. This feels like some knotty technical debt, but I think we have to live with it for now.

@seear
Copy link
Contributor

seear commented Feb 9, 2016

It is very tempting to introduce a new Elastic Search endpoint now to populate theme-details, because a new endpoint would be faster and have no caching issues. However, the new endpoint is a known, risk-free proposition, so let's use 1.1 for now and forge ahead.

@ockham
Copy link
Contributor Author

ockham commented Feb 9, 2016

A confusing thing at the moment is that this new theme-details section of state tree has less information than the existing themes section. This feels like some knotty technical debt, but I think we have to live with it for now.

I went with the basic stuff that's required for #2875, tough it's obviously easy to add more fields as returned by the endpoint. I was just thinking adding them as we move forward made more sense than adding too many now and pruning back later. That said, we can add stuff like screenshot, description, or description_long already...

@seear
Copy link
Contributor

seear commented Feb 9, 2016

I went with the basic stuff that's required for #2875

Yeah, that makes total sense. I'm more thinking of how do we answer the question, "what is the difference between themes and theme-details?"

@ockham
Copy link
Contributor Author

ockham commented Feb 9, 2016

themes-details is really meant to return information for a given theme (and that can be quite a lot of information to go over the network, see description_long), while themes fetches all themes for a given site, but with only the level of detail that is needed themes-listing pages.

I agree however that the separation of concerns could be clearer, but to me that's more about refactoring themes and themes-list subtrees.

@seear
Copy link
Contributor

seear commented Feb 9, 2016

but to me that's more about refactoring themes and themes-list subtrees.

yeah, totally agree

},

refresh( props ) {
if ( ! this.props.name && props.id ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the ! this.props.name part for here?

@seear
Copy link
Contributor

seear commented Feb 9, 2016

The amount of boilerplate required to add a few fields to the store is mildly alarming. Regardless, this is looking good to go 👍

@mcsf
Copy link
Member

mcsf commented Feb 10, 2016

Looking at this again: would it have been advantageous to reuse the themes reducer (and actions?) and have the new getThemeDetails return a subset of a theme's data? Complemented with a special hasThemeDetails that would look for specific fields, the data fetcher component would know when to trigger a fetch.

That said, I don't mind merging this right now and leaving all that to a latter and larger tree consolidation effort.

@seear seear force-pushed the add/theme-details-redux-store branch from 6b97af8 to 7cb9958 Compare February 12, 2016 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature Group] Appearance & Themes Features related to the appearance of sites.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants