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: Refactor page templates to use Redux state #7229

Merged
merged 5 commits into from
Aug 8, 2016

Conversation

aduth
Copy link
Contributor

@aduth aduth commented Aug 2, 2016

This pull request seeks to deprecate Flux-based page templates state in favor of global Redux-based state introduced in #7174. It introduces a new <QueryPageTemplates /> query component, refactoring the <EditorPageTemplates /> component to make use of it. Since this is the only area of the application using Flux-based state and the <PageTemplatesData /> component, both of these were removed.

Testing instructions:

Verify that page template options are shown and can be managed, especially with the following considerations:

Test live: https://calypso.live/?branch=update/editor-page-templates

@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. labels Aug 2, 2016
);
}

EditorDrawerLabel.propTypes = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice - This could be used in Tags & Categories 👍

@timmyc
Copy link
Contributor

timmyc commented Aug 4, 2016

Testing out well for me 🚢

@timmyc timmyc 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 Aug 4, 2016
@gwwar
Copy link
Contributor

gwwar commented Aug 4, 2016

👍 The code here looks good and I wasn't able to break this while testing.

@aduth aduth force-pushed the update/editor-page-templates branch from d04e317 to 34d71ab Compare August 5, 2016 13:16
@aduth
Copy link
Contributor Author

aduth commented Aug 5, 2016

Ran into a couple issues during final round of testing:

  • Looks like the previous <PageTemplatesData /> component was responsible for injecting the default template option, and this wasn't reflected with the original changes here. Default option should once again be available in the list as of c956700.
  • While current flows would not have surfaced this as an issue, page templates should be refetched if the theme changes for a site while the component is mounted. This is reflected in dcbda64.

To the latter point, this reminds me that I'd once considered perhaps keying page templates by theme rather than by site, since this would allow us to reuse the page templates between two sites both using the same theme. Looking into this some more, aside from potential conflicts on theme slug, there's also the issue that themes can be enhanced with more page templates on a per-site basis. For example, a standard WordPress.com site enables an "Eventbrite Events" page template for the Twenty Sixteen theme, which wouldn't be available for a Jetpack site using the same theme.

@aduth aduth force-pushed the update/editor-page-templates branch from 34d71ab to c956700 Compare August 5, 2016 13:38
@timmyc
Copy link
Contributor

timmyc commented Aug 5, 2016

Looks like the previous component was responsible for injecting the default template option, and this wasn't reflected with the original changes here.

Good catch - I knew there was a bug I had fixed in this component before and that was it. There are a few themes with only one page template, comics is one for example, and confirmed it is now properly showing the drop down with default and full-width as options 👍

While current flows would not have surfaced this as an issue, page templates should be refetched if the theme changes for a site while the component is mounted

Definitely an edge case, but maybe multiple tabs could be open to arrive at such a global state.

Tested again, still looking good 🚢

@aduth aduth force-pushed the update/editor-page-templates branch from c956700 to 6dc92e6 Compare August 8, 2016 13:52
@aduth aduth merged commit 222083f into master Aug 8, 2016
@aduth aduth deleted the update/editor-page-templates branch August 8, 2016 13:58
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants