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 current-theme query component #5361

Merged
merged 7 commits into from
May 13, 2016
Merged

Conversation

seear
Copy link
Contributor

@seear seear commented May 12, 2016

A query component to replace the old-style fetcher component. Will make it very easy to determine current theme properly in theme sheets (#5285)

No visual changes

screen shot 2016-05-13 at 11 56 20

To Test

  • Go to http://calypso.localhost:3000/design/
  • choose a site
    Expected: the CURRENT THEME bar works as previously
    Things to check:
  • switching sites
  • jetpack site
  • localStorage.clear(); in console then refresh
  • activate a theme

Replaces the old-style fetcher component.
@seear seear force-pushed the add/query-current-theme branch from 85aa01e to fcc292a Compare May 13, 2016 08:52
@seear seear 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 May 13, 2016
@ockham
Copy link
Contributor

ockham commented May 13, 2016

The (visual) current theme component isn't updated for me when activating a theme on a Jetpack site, and even the thanks modal display's the previous theme's name :-(

@ockham
Copy link
Contributor

ockham commented May 13, 2016

Actually, not just on Jetpack sites...

case THEME_ACTIVATE:
return state.set( 'isActivating', true );
case THEME_ACTIVATED:
return state
.set( 'isActivating', false )
.set( 'hasActivated', true )
.setIn( [ 'currentThemes', action.site.ID ], action.theme );
.setIn( [ 'currentThemes', action.siteId ], action.theme );
Copy link
Contributor Author

@seear seear May 13, 2016

Choose a reason for hiding this comment

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

@ockham: this is the problem—overzealous trimming of site usage. Fixing...

@seear
Copy link
Contributor Author

seear commented May 13, 2016

The (visual) current theme component isn't updated for me when activating a theme on a Jetpack site, and even the thanks modal display's the previous theme's name :-(

fixed in b2ba232

@ockham
Copy link
Contributor

ockham commented May 13, 2016

Loving it. Squash it, 🚢 it!

Aside — Can we incorporate ActivatingThemeData’s functionality into this component as a follow-up, and drop that one? :-)

@seear
Copy link
Contributor Author

seear commented May 13, 2016

Aside — Can we incorporate ActivatingThemeData’s functionality into this component as a follow-up, and drop that one? :-)

Yeah, was mulling that over myself, definite next step.

Thanks for review :)

@seear seear merged commit 3ede1a2 into master May 13, 2016
@seear seear deleted the add/query-current-theme branch May 13, 2016 14:05
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label May 13, 2016
roundhill pushed a commit that referenced this pull request May 17, 2016
Replaces the old-style fetcher component.
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. Framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants