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: Remove Flux stores #1363

Merged
merged 3 commits into from
Dec 16, 2015
Merged

Themes: Remove Flux stores #1363

merged 3 commits into from
Dec 16, 2015

Conversation

ockham
Copy link
Contributor

@ockham ockham commented Dec 8, 2015

To test:

  • Make sure the Themes showcase (calypso.localhost:3000/design) works as before.
  • Run make test inside shared/lib/themes

Changes:

  • reducers/themes-last-query: Remove Flux dispatcher waitFor
  • reducers/themes-list: Use isJetpack() from selectors instead of
    ThemeLastQueryStore's
  • Kill shared/lib/themes/stores
  • Remove obsolete helper functions from reducers, now that they are in selectors/
  • Default-export reducers, don't export initialStates anymore
  • Update README.md
  • Rewrite tests in shared/lib/themes to directly test reducers instead of stores
    Note that in test/themes-list-store, the Jetpack test is removed since filtering
    is no longer done in the reducer but in the themes-list-fetcher data component
    (using the getFilteredThemes()) now.

@ockham ockham added [Feature Group] Appearance & Themes Features related to the appearance of sites. [Status] In Progress labels Dec 8, 2015
@ockham ockham self-assigned this Dec 8, 2015
@ockham ockham added this to the Themes: Showcase M3-LoggedOut milestone Dec 8, 2015
@ockham ockham force-pushed the remove/themes-flux-stores branch 2 times, most recently from c22f768 to bd080bf Compare December 8, 2015 13:35
@ockham ockham mentioned this pull request Dec 8, 2015
2 tasks
@ockham ockham force-pushed the remove/themes-flux-stores branch 2 times, most recently from 5e40a47 to 515b418 Compare December 10, 2015 21:42
@ockham
Copy link
Contributor Author

ockham commented Dec 10, 2015

Note from the latest commit:

Remove ThemesListStore, update tests
Had to remove the Jetpack test since filtering is no longer done in
the reducer but in the themes-list-fetcher data component (using the
getFilteredThemes() selector) now.

* `reducers/themes-last-query`: Remove Flux dispatcher `waitFor`
* `reducers/themes-list`: Use `isJetpack()` from selectors instead of
  `ThemeLastQueryStore`'s
* Kill `shared/lib/themes/stores`
* Remove obsolete helper functions from reducers, now that they are in `selectors/`
* Default-export reducers, don't export initialStates anymore
* Update README.md
* Rewrite tests in `shared/lib/themes` to directly test reducers instead of stores
  Note that in test/themes-list-store, the Jetpack test is removed since filtering
	is no longer done in the reducer but in the themes-list-fetcher data component
  (using the getFilteredThemes()) now.
@ockham ockham force-pushed the remove/themes-flux-stores branch from a03745f to 51b1aef Compare December 16, 2015 17:12
@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 Dec 16, 2015
@mcsf
Copy link
Member

mcsf commented Dec 16, 2015

Code-wise, looks good!

@mcsf
Copy link
Member

mcsf commented Dec 16, 2015

Feels good to slash code. :shipit:

ockham added a commit that referenced this pull request Dec 16, 2015
@ockham ockham merged commit 1a9298b into master Dec 16, 2015
@scruffian scruffian removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 16, 2015
@ockham ockham deleted the remove/themes-flux-stores branch December 16, 2015 20:32
@ockham
Copy link
Contributor Author

ockham commented Dec 16, 2015

Thanks!

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.

3 participants