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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactored feature theme mods #571

Open
wants to merge 14 commits into
base: develop
from

Conversation

Projects
None yet
2 participants
@kidunot89
Copy link
Contributor

commented Nov 5, 2018

Your checklist for this pull request

Thanks for sending a pull request! Please make sure you click the link above to view the contribution guidelines, then fill out the blanks below.

馃毃Please review the guidelines for contributing to this repository.

  • Make sure you are making a pull request against the develop branch (left side). Also you should start your branch off our develop.
  • Make sure you are requesting to pull request from a topic/feature/bugfix branch (right side). Don't pull request from your master!

What does this implement/fix? Explain your changes.

Implements ThemeMods queries and mutations. Tests included.

Does this close any currently open issues?

#513

Any relevant logs, error output, GraphiQL screenshots, etc?

(If it鈥檚 long, please paste to https://ghostbin.com/ and insert the link here.)

Any other comments?

Features:

  • Refactored for v0.1.x - register_* API
  • Six default theme mods are defined, background, backgroundColor, customCssPostId, customLogo, headerImage, and navMenuLocations.
  • Filters can be used to define more theme mods.

New Issues:

  • Edge data in header_image and background_image mods stored and can be updated in mutations but can not be queried at the moment.
  • customCSSPost is a complex type and not a simple WP_Post much like Attachment. It should have a type definition of its own.

Where has this been tested?

Operating System: Ubuntu 18.04

WordPress Version: 4.9.8

$values = [];
$locations = array_keys( get_nav_menu_locations() );
$locations = DataSource::get_registered_nav_menu_locations();

This comment has been minimized.

Copy link
@kidunot89

kidunot89 Nov 5, 2018

Author Contributor

Gets all nav menu locations even ones that don't have menus assigned.

@codecov

This comment has been minimized.

Copy link

commented Nov 5, 2018

Codecov Report

Merging #571 into develop will increase coverage by 0.31%.
The diff coverage is 66.79%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #571      +/-   ##
===========================================
+ Coverage    58.95%   59.26%   +0.31%     
===========================================
  Files          105      111       +6     
  Lines         6500     6768     +268     
===========================================
+ Hits          3832     4011     +179     
- Misses        2668     2757      +89
Impacted Files Coverage 螖
src/Type/Enum/MenuLocationEnum.php 66.66% <100%> (酶) 猬嗭笍
src/TypeRegistry.php 77.87% <100%> (+0.32%) 猬嗭笍
src/Type/Input/CustomBackgroundInput.php 50% <50%> (酶)
src/Type/Input/CustomHeaderInput.php 52.63% <52.63%> (酶)
src/Mutation/UpdateThemeMods.php 57.44% <57.44%> (酶)
src/Type/Object/ThemeMods.php 61.19% <61.19%> (酶)
src/Data/DataSource.php 80.37% <71.42%> (-1.69%) 猬囷笍
src/Data/ThemeModsMutation.php 82.97% <82.97%> (酶)
src/Type/Input/NavMenuLocationsInput.php 91.66% <91.66%> (酶)
... and 6 more

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 e432480...95e80d2. Read the comment docs.

@@ -4,12 +4,14 @@ services:
wpgraphql.test:
image: "wordpress:${WP_VERSION}-php${PHP_VERSION}-apache"
ports:
- '80:80'
- '8000:80'

This comment has been minimized.

Copy link
@jasonbahl

jasonbahl Nov 5, 2018

Collaborator

can we get this as a separate PR?

environment:
WORDPRESS_DB_HOST: 'mysql_test'
WORDPRESS_DB_NAME: 'wpgraphql_test'
WORDPRESS_DB_USER: 'root'
WORDPRESS_DB_PASSWORD: 'testing'
WORDPRESS_CONFIG_EXTRA: |

This comment has been minimized.

Copy link
@jasonbahl

jasonbahl Nov 5, 2018

Collaborator

^ see prev comment about separate PR

@@ -6,4 +6,96 @@
$baseDir = dirname($vendorDir);
return array(
'CLI_Command' => $vendorDir . '/wp-cli/wp-cli/php/commands/src/CLI_Command.php',

This comment has been minimized.

Copy link
@jasonbahl

jasonbahl Nov 5, 2018

Collaborator

@kidunot89 try running composer install --no-dev and committing again. This is trying to add all the dev dependencies as well

@jasonbahl
Copy link
Collaborator

left a comment

@kidunot89 couple quick changes, but this is looking 馃敟

if ( ! empty( $locations ) ) {
foreach( $locations as $location ) {
$fields[ $location ] = [
'type' => 'Int',

This comment has been minimized.

Copy link
@jasonbahl

jasonbahl Nov 5, 2018

Collaborator

let's make the Type ID

return ( ! empty( $root['custom_css_post_id'] ) ) ? $root['custom_css_post_id'] : null;
}
],
'customLogo' => [

This comment has been minimized.

Copy link
@jasonbahl

jasonbahl Nov 5, 2018

Collaborator

This should maybe be a more complex type than Int. . .like a MediaItem perhaps?

This comment has been minimized.

Copy link
@jasonbahl

jasonbahl Nov 5, 2018

Collaborator

Hmm. If we're going to expose the PostObjects for the custom_css post_type, we probably should register the custom_css as show_in_graphql => true and give it a graphql_single_name and graphql_plural_name.

Then, instead of resolving to type => Post we can resolve to type => CustomCSS or something to that tune.

And the resolver should also be changed to DataSource::resolve_post_object( absint( $root['custom_css_post_id'] ), 'custom_css' )

Right now it's misleading that CSS is a "post" when really it's not, especially when the Schema is filtered by various plugins to shape the Schema more for their particular applications.

The CustomCSS PostObject Type should have the ability to be shaped differently than the Post type in the Schema.

This comment has been minimized.

Copy link
@kidunot89

kidunot89 Nov 5, 2018

Author Contributor

I agree, but I thought it should have a separate issue and branch like the handling of the custom-background and custom-header context data issue I mentioned above.

This comment has been minimized.

Copy link
@jasonbahl

jasonbahl Nov 5, 2018

Collaborator

馃

I don't want to intentionally add this to resolve to Post if we know it's for sure going to be a breaking change to make the switch to resolve to CustomCSS later.

This comment has been minimized.

Copy link
@kidunot89

kidunot89 Nov 5, 2018

Author Contributor

I'll resolve to ID for now and make an new issue for a CustomCSS post-object type

This comment has been minimized.

Copy link
@jasonbahl

jasonbahl Nov 5, 2018

Collaborator

馃

Hmm. . .

I dunno the best approach here.

If we expose the field customCssPostId and just return the ID, I suppose that might be ok for now, then come back and add a customCSS field that resolves as an object of the CustomCSS type that could work.

\Codeception\Util\Debug::debug( attachment_url_to_postid( $mod_data ) );
$theme_mod_data[ 'header_image' ]['id'] = attachment_url_to_postid( (string) $mod_data );
break;

This comment has been minimized.

Copy link
@kidunot89

kidunot89 Nov 5, 2018

Author Contributor

Edge data in header_image and background_image mods stored and can be updated in mutations but can not be queried at the moment.

return null;
},
],
'navMenu' => [

This comment has been minimized.

Copy link
@kidunot89

kidunot89 Nov 5, 2018

Author Contributor

Edge data in header_image and background_image mods stored and can be updated in mutations but can not be queried at the moment.

kidunot89 and others added some commits Nov 5, 2018

customCssPostId<Int> => customCssPost<Post> and customLogo<Int> => cu鈥
鈥tomLogo<MediaItem>. UpdateThemeMods input field customCssPostId renamed to customCssPost.
Merge pull request #572 from kidunot89/docker-local-app-patch
local-app file upload limit size pushed to 64MB
customCssPostId<Int> => customCssPost<Post> and customLogo<Int> => cu鈥
鈥tomLogo<MediaItem>. UpdateThemeMods input field customCssPostId renamed to customCssPost.
'type' => 'Int',
'description' => __( 'WP ID of customLogo mediaItem' ),
'type' => 'MediaItem',
'description' => __( 'Site Custom Logo' ),

This comment has been minimized.

Copy link
@jasonbahl

jasonbahl Nov 5, 2018

Collaborator

missing textdomain in the translation

'description' => __( 'custom theme logo' ),
'customCssPost' => [
'type' => 'Post',
'description' => __( 'WP Post storing theme custom CSS' ),

This comment has been minimized.

Copy link
@jasonbahl

jasonbahl Nov 5, 2018

Collaborator

missing textdomain in the translation

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