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

Update to latest library version from Packagist #3

Merged
merged 3 commits into from
Sep 24, 2024
Merged

Conversation

felixarntz
Copy link
Collaborator

Now that Third Party Capital is on Packagist, this PR updates to its latest version coming from there.

It also updates the readme file, adding an example for the recently added Google Tag Manager third party, and it adds a few other minor readme enhancements like linking to the JSON schema source files for additional information on the supported parameters. This is probably fine at this point, as from the JSON they're easy enough to understand for developers, and it means that we don't have to manually maintain documentation for all the fields in here.

@felixarntz felixarntz added the enhancement New feature or request label Sep 23, 2024
@felixarntz
Copy link
Collaborator Author

@adamsilverstein Once this is merged, I think the codebase is ready for a 1.0 release, then only pending a first Packagist release of Third Party Capital. I believe the idea is to release its version 3.0 soon, and once that is out we can rely on it here and ship wp-third-parties 1.0.

)
);

add_action( 'wp_loaded', array( $gtm, 'add_hooks' ) );
Copy link
Collaborator

Choose a reason for hiding this comment

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

reviewing the entire doc I noticed they all include a hook line like add_action( 'wp_loaded', array( $yte, 'add_hooks' ) ); except "Google Maps Embed". Should it also include the hook line? If not maybe a single line explaining why these are included in the other examples but not this one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@adamsilverstein Great point! I clarified this in 7c43462.

The reason Google Maps Embed doesn't need to add hooks is that there are no assets (scripts or stylesheets) needed for it. You could technically still include the same hook callback, but it just wouldn't do anything. So if you know that already, you can omit it, that's why the snippet here does that.

Copy link
Collaborator

@adamsilverstein adamsilverstein left a comment

Choose a reason for hiding this comment

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

Looks good! One non-blocking question.

@felixarntz felixarntz merged commit f16bdfd into main Sep 24, 2024
3 checks passed
@felixarntz felixarntz deleted the update/library branch September 25, 2024 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants