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

refactor!: nuxt 3 only support #186

Merged
merged 17 commits into from
Oct 2, 2023
Merged

refactor!: nuxt 3 only support #186

merged 17 commits into from
Oct 2, 2023

Conversation

kylegl
Copy link
Contributor

@kylegl kylegl commented Sep 24, 2023

In this PR, I have rewritten the Nuxt 3 component using the Composition API. To ensure type safety, I had to remove the Nuxt 2 module. I propose that we handle this similarly to Nuxt Image by maintaining two separate branches: v0 for Nuxt 2 and main for Nuxt 3.

There are few notable changes:

  • The module name in the Nuxt config has been changed from google-adsense to googleAdsense. This adjustment was made to resolve type errors caused by the hyphen.
  • The module config now accepts a new option called hideUnfilled. This option will hide any unfilled ads, as recommended in the Google AdSense documentation.
  • The adFormat component prop no longer defaults to auto. I encountered scenarios while using the module where I needed adFormat to be undefined, and this caused issues. A default global adFormat can be set in the module config options and overridden with component props if needed.
  • The component now exposes the showAd and updateAd methods, as well as an isUnfilled computed ref for user customization if needed.

This PR fixes issue #174 #173

chore: remove nuxt2 compatibility

docs: update readme
chore: cleanup

fix: ad client

chore: cleanup

fix: module client id check

refactor: adsense computed properties

style: layout

feat: update ad on query change

feat: option to hide unfilled ads

chore: expose composable methods from component

fix: hideUnfilled global

chore: remove default ad format
@kylegl
Copy link
Contributor Author

kylegl commented Sep 25, 2023

@manniL

@manniL manniL self-requested a review September 25, 2023 07:05
@manniL
Copy link
Collaborator

manniL commented Sep 25, 2023

@kylegl Thanks for the PR! Good work 👏

This means that the module is not Nuxt 2 compatible anymore, right? Hence we'd need a new major version (as mentioned in the updated README).

@@ -130,14 +152,24 @@ to handle updating ads on progressive web applications:

Each time a new advertisement is requested, the AdSense parameter `data-ad-region` is
updated to a random value. For this reason, you cannot set the `data-ad-region` attribute
on the `<adsbygoogle />` component.

For test mode, the following blog was used as a reference:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is still applied in the component, right?

Copy link
Contributor Author

@kylegl kylegl Sep 25, 2023

Choose a reason for hiding this comment

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

Yes it is still applied. I just capitalized the component name and removed the link to the blog that 404s here.

package.json Outdated
@@ -1,12 +1,13 @@
{
"name": "@nuxtjs/google-adsense",
"version": "2.1.0",
"version": "1.0.0",
Copy link
Collaborator

@manniL manniL Sep 25, 2023

Choose a reason for hiding this comment

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

Probably has to go to 3.x then

Suggested change
"version": "1.0.0",
"version": "3.0.0",

@kylegl
Copy link
Contributor Author

kylegl commented Sep 25, 2023

@kylegl Thanks for the PR! Good work 👏

This means that the module is not Nuxt 2 compatible anymore, right? Hence we'd need a new major version (as mentioned in the updated README).

Thanks!

Correct, this pr removes Nuxt 2 compatibility. I wanted to get your thoughts on how to handle this.

Did you happen to see how the Nuxt Image module handled dropping Nuxt 2? They made a branch for Nuxt 2 and branch for Nuxt 3. Then to install Nuxt 2 module is pnpm install @nuxt/image and to install Nuxt 3 is @nuxt/ image@rc.

I think having separate branches might be better because the docs & config are different, but I'm not sure the best way to handle it.
@manniL I'd be happy to set this up. lmk if I can help in anyway.

@manniL
Copy link
Collaborator

manniL commented Oct 2, 2023

Correct, this pr removes Nuxt 2 compatibility. I wanted to get your thoughts on how to handle this.

I think having separate branches might be better because the docs & config are different, but I'm not sure the best way to handle it. @manniL I'd be happy to set this up. lmk if I can help in anyway.

I think the easiest is mentioning that the Nuxt 2 version can be found under branch XYZ and the package only supports Nuxt 3 from major version ABC on.

That shouldn't be an issue at all 👍

CHANGELOG.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@manniL
Copy link
Collaborator

manniL commented Oct 2, 2023

@kylegl created a legacy-v2 branch and annotated some more things! Please tag for a re-review, then I think it is good to merge 👍

kylegl and others added 6 commits October 2, 2023 02:12
Co-authored-by: Alexander Lichter <github@lichter.io>
Co-authored-by: Alexander Lichter <github@lichter.io>
Co-authored-by: Alexander Lichter <github@lichter.io>
Co-authored-by: Alexander Lichter <github@lichter.io>
Co-authored-by: Alexander Lichter <github@lichter.io>
Co-authored-by: Alexander Lichter <github@lichter.io>
@kylegl
Copy link
Contributor Author

kylegl commented Oct 2, 2023

Excellent, everything looks good to go!

@kylegl kylegl requested a review from manniL October 2, 2023 09:18
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@manniL manniL changed the title refactor: nuxt 3 component composition api refactor: nuxt 3 only support Oct 2, 2023
@manniL manniL changed the title refactor: nuxt 3 only support refactor!: nuxt 3 only support Oct 2, 2023
@manniL manniL merged commit 4be3f96 into nuxt-modules:master Oct 2, 2023
This was referenced Oct 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants