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

✨ Add TimezoneEnum Type #676

Merged
merged 2 commits into from Mar 18, 2019

Conversation

Projects
None yet
2 participants
@jasonbahl
Copy link
Collaborator

commented Jan 30, 2019

This adds the Type definition, but doesn't define any field to use it yet. . .this is to support the Settings Patch and get the settings all in "proper" order

Closes #675

ADD TIMEZONE ENUM
This adds the Type definition, but doesn't define any field to use it yet. . .this is to support the Settings Patch and get the settings all in "proper" order

Closes #675

@jasonbahl jasonbahl added this to the 0.2.2 milestone Jan 30, 2019

@jasonbahl jasonbahl self-assigned this Jan 30, 2019

@jasonbahl jasonbahl changed the title ✨ ADD TIMEZONE ENUM ✨ Add Timezone Enum Jan 30, 2019

@jasonbahl jasonbahl changed the title ✨ Add Timezone Enum ✨ Add TimezoneEnum Type Jan 30, 2019

@codecov

This comment has been minimized.

Copy link

commented Jan 30, 2019

Codecov Report

Merging #676 into develop will decrease coverage by 0.22%.
The diff coverage is 47.93%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #676      +/-   ##
===========================================
- Coverage    60.47%   60.25%   -0.23%     
===========================================
  Files          109      110       +1     
  Lines         6682     6803     +121     
===========================================
+ Hits          4041     4099      +58     
- Misses        2641     2704      +63
Impacted Files Coverage Δ
src/TypeRegistry.php 79.05% <100%> (+0.05%) ⬆️
src/Type/Enum/TimezoneEnum.php 47.5% <47.5%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 23147c2...1709e09. Read the comment docs.

@CodeProKid
Copy link
Member

left a comment

Looks mostly good. One piece of feedback, also what are your thoughts on changing the coding style to match the plugin (ie. [] instead of array()) for chunks of code copied straight from core?

// Intentionally avoid WPEnumType::get_safe_name here for specific timezone formatting
$enum_values[ $offset_name ] = [
'value' => $offset_value,
'description' => __( sprintf( 'UTC offset: %s', $offset_name ), 'wp-graphql' )

This comment has been minimized.

Copy link
@CodeProKid

CodeProKid Jan 31, 2019

Member

sprintf should wrap the __() function so the string doesn't have to be translated multiple times.

@jasonbahl jasonbahl modified the milestones: 0.2.2, 0.2.3 Feb 26, 2019

@jasonbahl

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 26, 2019

@CodeProKid moved this to 0.2.3 🤷‍♂️

@jasonbahl jasonbahl merged commit 4a795fd into wp-graphql:develop Mar 18, 2019

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

This was referenced Mar 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.