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

Feature/#381 - Selected Terms in TermObjectConnection queries #387

Conversation

Projects
None yet
6 participants
@jasonbahl
Copy link
Collaborator

commented Feb 7, 2018

Your checklist for this pull request

  • 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.

This fixes term connection fields on PostObjects, to allow for flexibility in querying only "selected" items, and displaying in a flat list vs. hierarchical list.

This adds 2 new arguments to TermObjectConnection fields:

shouldOnlyIncludeConnectedItems and shouldOutputInFlatList

  • shouldOnlyIncludeConnectedItems: Default behavior is true. If true, this ensures that only items connected to the root post being queried will be returned. If false, this will output all terms, regardless of connection. Use cases would be an Admin Page for editing a post. To hydrate initially, you might want to query all terms, using shouldOnlyIncludeConnectedItems: false, then on subsequent fetches you might only want to re-fetch selected terms, shouldOnlyIncludeConnectedItems: true
  • shouldOutputInFlatList: Default behavior is false. If true, this outputs the terms in a flat list, instead of hierarchical. So parent and children terms will be output in the list of nodes instead of having to ask for children. Sometimes you want your query structure to match your Dom Tree, and be hierarchical, but sometimes you don't, so this gives some flexibility to that.

Does this close any currently open issues?

closes #381

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

screen shot 2018-02-07 at 2 31 04 pm
screen shot 2018-02-07 at 2 32 28 pm

screen shot 2018-02-07 at 2 33 18 pm

screen shot 2018-02-07 at 3 37 52 pm

Other notes?

This is technically a breaking change, however it's relatively minor.

The input types for connection arguments have changed, so if you used variables in your queries before, they will need to be updated to reflect the change.

You can check the changes in this PR that were made to the test files to see how minor the changes were to have unit tests pass still.

Where has this been tested?

Operating System:
Mac OSX v10.12.6

WordPress Version:
4.9.4

jasonbahl added some commits Feb 7, 2018

#381 - selected terms on postobjects
- Making user connection dynamic
#381 - selected terms on postobjects
- Making user connection dynamic
- Making comment connections dynamic
#381 - selected terms on postobjects
- Making theme connection dynamic
- Making plugin connection dynamic
#381 - selected terms on postobjects
- Making term object connections dynamic
#381 - selected terms on postobjects
- Making post object connections dynamic
- Making user connection args dynamic
#381 - selected terms on postobjects
- Making post object connections dynamic
- Making user connection args dynamic

@jasonbahl jasonbahl changed the title Feature/#381 selected post terms redux Feature/#381 - Selected Terms in TermObjectConnection queries Feb 7, 2018

@coveralls

This comment has been minimized.

Copy link

commented Feb 7, 2018

Coverage Status

Coverage increased (+0.1%) to 91.52% when pulling d92db9c on jasonbahl:feature/#381-selected-post-terms-redux into 47ee3b6 on wp-graphql:develop.

@cr101

This comment has been minimized.

Copy link

commented Feb 8, 2018

@jasonbahl For example if have the post categories below:
aciform
-sub (selected)
--sub1
---sub2 (selected)

I only want to display the selected categories on the front-end. How can I only fetch the selected categories (sub and sub2) without the parent (aciform)?

@hughdevore
Copy link
Collaborator

left a comment

@jasonbahl, this looks great. Seeing it in code got me on the same page but the screenshots helped me see the use case. 👍

@ginatrapani

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2018

Looks great! This is very exciting!

@CodeProKid
Copy link
Member

left a comment

@jasonbahl a couple of things here. Mostly just need to cleanup some of the docblocs, and then that question about setting the class var to an array from the start so we don't have to do it in the method.

return self::$args[ $connection ];

This comment has been minimized.

Copy link
@CodeProKid

CodeProKid Feb 8, 2018

Member

Extra blank line here

if ( null === self::$connection ) {
self::$connection = [];

This comment has been minimized.

Copy link
@CodeProKid

CodeProKid Feb 8, 2018

Member

Rather than doing this, wouldn't it make sense to have self::$connection have a default value of [] and then just check if it's empty like you are on like 35?

}
return self::$connection;
endif;

This comment has been minimized.

Copy link
@CodeProKid

CodeProKid Feb 8, 2018

Member

Should probably use brackets for consistency, eh?

@@ -51,12 +51,12 @@ class PostObjectConnectionArgs extends WPInputObjectType {
/**
* PostObjectConnectionArgs constructor.
*
* @param string $connection

This comment has been minimized.

Copy link
@CodeProKid

CodeProKid Feb 8, 2018

Member

Should also probably have a @param for $config here as well. Some details here would also be helpful.

@@ -35,27 +35,27 @@ class PostObjectConnectionDefinition {
* Method that sets up the relay connection for post objects
*
* @param object $post_type_object
*
* @param string $from_type

This comment has been minimized.

Copy link
@CodeProKid

CodeProKid Feb 8, 2018

Member

Some more details here on what $from_type is would be helpful.

public static function connection( $from_type = 'Root' ) {
if ( null === self::$connection ) {
self::$connection = [];

This comment has been minimized.

Copy link
@CodeProKid

CodeProKid Feb 8, 2018

Member

Same as above comments

@@ -25,20 +25,24 @@ class ThemeConnectionDefinition {
/**
* Method that sets up the relay connection for term objects
*
* @param string $from_type

This comment has been minimized.

Copy link
@CodeProKid

CodeProKid Feb 8, 2018

Member

incomplete

@@ -40,11 +40,13 @@ class UserConnectionArgs extends WPInputObjectType {
/**
* UserConnectionArgs constructor.
* @param array $config
* @param string $connection

This comment has been minimized.

Copy link
@CodeProKid

CodeProKid Feb 8, 2018

Member

Incomplete

if ( null === self::$connection ) :
if ( null === self::$connection ) {
self::$connection = [];

This comment has been minimized.

Copy link
@CodeProKid

CodeProKid Feb 8, 2018

Member

Same as above comments

@@ -709,7 +740,7 @@ public static function list_of( $type ) {
/**
* This is a wrapper for the GraphQL type to give a consistent experience
*
* @param callable $type instance of GraphQL\Type\Definition\Type or callable returning instance
* @param object $type instance of GraphQL\Type\Definition\Type or callable returning instance

This comment has been minimized.

Copy link
@CodeProKid

CodeProKid Feb 8, 2018

Member

my phpcs approves this change 👍

@jasonbahl

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 8, 2018

@cr101 You should be able to get the selected items by setting both new arguments to true, like:

{
  post(id:"...") {
    categories( shouldOnlyIncludeConnectedItems: true,  shouldOutputInFlatList: true ) {
       nodes {
         id
         name
       }
     }
  }
}

You can see the query returning the 3 selected categories, but not the unselected one.

Here's some screenshots showing this:

screen shot 2018-02-08 at 9 53 34 am
screen shot 2018-02-08 at 9 52 34 am

@jasonbahl

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 8, 2018

@CodeProKid I pushed updates addressing what you pointed out in your review. Additionally, I swept through for conditional blocks using : and endif and replaced with brackets.

@CodeProKid CodeProKid merged commit 5fbaa1c into wp-graphql:develop Feb 8, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.1%) to 91.52%
Details

This was referenced Feb 8, 2018

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.