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

feat: add missing extra fields to EnqueuedAsset, EnqueuedScript and EnqueuedStylesheet #2988

Merged

Conversation

justlevine
Copy link
Collaborator

@justlevine justlevine commented Nov 10, 2023

What does this implement/fix? Explain your changes.

The PR registers GraphQL fields for script/style data stored in the $extra property on the instance. More specifically:

  • EnqueuedAsset
    • added after and before fields, for code that is supposed to be inlined before/after the asset is called.
    • added conditional field, for HTML comment conditionals
    • deprecated extra in favor of EnqueuedScript.extraData.
    • deprecated args in favor of EnqueuedStylesheet.media.
    • fixed dependency to be an EnqueuedAsset instead of an EnqueuedScipt.
  • EnqueuedScript
    • added extraData for inline js data passed to the script (previously `EnqueuedAsset.extra)
    • added strategy and the corresponding ScriptLoadingStrategyEnum.
  • EnqueuedStylesheet
    • Added isRtl, media path, suffix, and title
    • Added rel, which is derived from $style->extra['alt'] since it seemed clunky to use an isAlt to force users to know they need to do rel={isAlt ? 'alternative stylesheet' : 'stylesheet' }

Does this close any currently open issues?

Fixes #2938

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

Any other comments?

Where has this been tested?

Operating System: Ubunut 20.04 (wsl2 + devilbox + php 8.1.15)

WordPress Version: 6.4.1

@justlevine justlevine added Type: Enhancement New feature or request Status: In Review Needs to be reviewed by a project maintainer before merge labels Nov 10, 2023
src/Type/ObjectType/EnqueuedStylesheet.php Show resolved Hide resolved
src/Type/ObjectType/EnqueuedStylesheet.php Show resolved Hide resolved
src/Type/ObjectType/EnqueuedScript.php Outdated Show resolved Hide resolved
src/Type/InterfaceType/EnqueuedAsset.php Show resolved Hide resolved
src/Type/InterfaceType/EnqueuedAsset.php Show resolved Hide resolved
@justlevine justlevine added the Needs: Tests Tests should be added to ensure this works as expected label Nov 10, 2023
src/Type/ObjectType/EnqueuedStylesheet.php Show resolved Hide resolved
src/Type/InterfaceType/EnqueuedAsset.php Show resolved Hide resolved
src/Type/InterfaceType/EnqueuedAsset.php Show resolved Hide resolved
src/Type/InterfaceType/EnqueuedAsset.php Outdated Show resolved Hide resolved
src/Type/InterfaceType/EnqueuedAsset.php Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Nov 10, 2023

Coverage Status

coverage: 84.792% (+0.07%) from 84.724%
when pulling 5eb6c08 on justlevine:feat/enqueued-asset-extra-fields
into 463f774 on wp-graphql:develop.

@justlevine justlevine force-pushed the feat/enqueued-asset-extra-fields branch from f1b5a72 to 40023c5 Compare November 10, 2023 23:39
@justlevine justlevine removed the Needs: Tests Tests should be added to ensure this works as expected label Nov 10, 2023
jasonbahl
jasonbahl previously approved these changes Nov 14, 2023
'src' => [
'resolve' => static function ( \_WP_Dependency $script ) {
return ! empty( $script->src ) && is_string( $script->src ) ? $script->src : null;
'extraData' => [
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 3 locations. Consider refactoring.

return $script->extra['data'];
},
],
'strategy' => [
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 3 locations. Consider refactoring.

return array_filter( $before_scripts );
},
],
'conditional' => [
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 3 locations. Consider refactoring.

'type' => [ 'list_of' => 'EnqueuedScript' ],
'description' => __( 'Dependencies needed to use this asset', 'wp-graphql' ),
],
'extraData' => [
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 3 locations. Consider refactoring.

return $script->extra['data'];
},
],
'strategy' => [
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 3 locations. Consider refactoring.

Copy link

codeclimate bot commented Nov 14, 2023

Code Climate has analyzed commit 5eb6c08 and detected 22 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 15
Duplication 7

View more on Code Climate.

@jasonbahl jasonbahl merged commit 3d284db into wp-graphql:develop Nov 14, 2023
25 of 28 checks passed
@jasonbahl jasonbahl mentioned this pull request Nov 14, 2023
@justlevine justlevine deleted the feat/enqueued-asset-extra-fields branch February 10, 2024 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: In Review Needs to be reviewed by a project maintainer before merge Type: Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong data being returned from EnqueuedAsset.extra resolve function
3 participants