-
Notifications
You must be signed in to change notification settings - Fork 442
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
feat: add WPGraphQL IDE as an "Experiment" #3144
feat: add WPGraphQL IDE as an "Experiment" #3144
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR diff size of 134975 lines exceeds the maximum allowed for the inline comments feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only implementation suggestion (until we add more Admin-related stuff in the API) is to colocate Ide
in Experimental\Experiment\WPGraphQLIDE
.
This would theoretically allow us to split up that 600-line class, but more importantly it makes it easy at a glance to determine what code is related to an experiment vs what code is mainlined into core.
src/Admin/Ide/Ide.php
Outdated
if ( function_exists( 'get_current_screen' ) ) { | ||
$screen = get_current_screen(); | ||
if ( 'toplevel_page_graphiql-ide' === $screen->id ) { | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return; | |
if ( null !== $screen && 'toplevel_page_graphiql-ide' === $screen->id ) { | |
return; |
src/Admin/Ide/Ide.php
Outdated
* @return mixed The modified value of the field. | ||
*/ | ||
public function ensure_graphiql_link_is_unchecked( $value, $default_value, string $option_name, array $section_fields, string $section_name ) { | ||
if ( 'show_graphiql_link_in_admin_bar' === $option_name && 'graphql_general_settings' === $section_name ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why $section_fields
can be a string
, but since we dont need it, easy enough to ignore the type.
if ( 'show_graphiql_link_in_admin_bar' === $option_name && 'graphql_general_settings' === $section_name ) { | |
public function ensure_graphiql_link_is_unchecked( $value, $default_value, string $option_name, $section_fields, string $section_name ) { |
src/Admin/Ide/Ide.php
Outdated
$plugin_data = get_plugin_data( __FILE__ ); | ||
|
||
return $plugin_data[ $key ] ?? null; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PHPStan: lets enforce the string return type.
} | |
return isset( $plugin_data[ $key ] ) ? (string) $plugin_data[ $key ] : null; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why
$section_fields
can be astring
, but since we dont need it, easy enough to ignore the type.
public function ensure_graphiql_link_is_unchecked( $value, $default_value, string $option_name, $section_fields, string $section_name ) {
This suggestion should be on the previous line 🤦
…eriment/wpgraphql-ide
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR diff size of 151858 lines exceeds the maximum allowed for the inline comments feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR diff size of 31863 lines exceeds the maximum allowed for the inline comments feature.
- remove built files from WPGraphQLIDE experiment (they shouldn't be versioned)
49760fd
to
9681ade
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR diff size of 31870 lines exceeds the maximum allowed for the inline comments feature.
Code Climate has analyzed commit 9681ade and detected 49 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
I'm closing this PR as I don't believe the IDE makes sense as an Experiment. The IDE is a full product on its own that deserves its own release lifecycle. The Experiments API is still something we want to add to core WPGraphQL and will benefit features such as:
These features are smaller in scope and are intended to directly relate to core WPGraphQL, but will require some iteration and feedback before being merged to the core codebase. Once merged, there's high likelihood there would be little iteration of these features, where the IDE will likely have continuous iteration long-term. |
Note to future selves: If a feature is in a very active state of development - defined as "likely requires more frequent public releases than WPGraphQL core's release cadence of |
This uses the beta "Experiments API" to register the new WPGraphQL IDE as an "Experiment"
WIP