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

State: Add timezones selectors functions #10932

Merged
merged 1 commit into from Jan 25, 2017

Conversation

retrofox
Copy link
Contributor

@retrofox retrofox commented Jan 25, 2017

This patch adds three state selectors and it's the first step of a set of PRs which improve the current implement of Timezones regarding the data approach, component, etc.

Testing

Beyond the app should work ok, you would run the selectors tests:

> npm run test-client client/state/selectors/test/index.js

NOTE: Selectors tests are already done (6814cff0d231fcab4233aa29c8bd9e1faa27013e), but they will be added in another PR here: #10941. The whole work related with the timezones improvements is here: #10788. You could take a look to see the trace of this process.

@retrofox retrofox added State [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 25, 2017
@retrofox retrofox requested a review from gwwar January 25, 2017 14:03
@matticbot
Copy link
Contributor

Copy link
Member

@obenland obenland left a comment

Choose a reason for hiding this comment

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

Was ab1d23d5e5096cb16f64bc74952f9fceb3e28282 supposed to be part of this PR? The action types seem unrelated to the selectors.

Is there a reason you have the selector tests separated out in a different PR? Are they dependent on something other than these selectors?

@retrofox
Copy link
Contributor Author

retrofox commented Jan 25, 2017

Was ab1d23d supposed to be part of this PR? The action types seem unrelated to the selectors.

Yes, It's a mistake. I should remove this commit from this PR

Is there a reason you have the selector tests separated out in a different PR? Are they dependent on something other than these selectors?

The only one reason is that these tests use a fixture file which is located into the state/timezone folder since it is used for testing either in these selectors as well as for the redux implementation.

This commit 6814cff0d231fcab4233aa29c8bd9e1faa27013e adds the selectors tests:

import { MANUAL_UTC_OFFSETS } from 'state/timezones/test/fixture';

I could create a transition commit if we consider necessary, though.

@retrofox retrofox force-pushed the update/timezones-state-selectors branch from f1218c7 to 2e3f479 Compare January 25, 2017 17:24
@retrofox retrofox force-pushed the update/timezones-state-selectors branch from 2e3f479 to 1363327 Compare January 25, 2017 17:27
@retrofox
Copy link
Contributor Author

@obenland In this PR #10934 I'm adding the fixture file to run the redux tests. I split up selectors and redux implementation in order to make shorter PRs.

So once #10934 is merged we will ale to add the selectors tests( WIP PR #10941)

Copy link
Member

@obenland obenland left a comment

Choose a reason for hiding this comment

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

Yeah, no need to get fancy, looks good from my end!

@retrofox
Copy link
Contributor Author

Thanks @obenland for the review. Is it ready to merge?

@obenland obenland 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 Jan 25, 2017
@retrofox retrofox merged commit afa58a1 into master Jan 25, 2017
@lancewillett lancewillett deleted the update/timezones-state-selectors branch January 30, 2017 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants