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

Add support to use ads.txt in the root #509

Draft
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

mdesignerco
Copy link

@mdesignerco mdesignerco commented Jun 30, 2020

What:

Added a check if an ads.txt exists in the root of the theme

Why:

Empowers creators of the content to add the support of ads.txt for google adsense

How:

I copied the line for favicon and i replaced with ads.txt

Tasks:

  • Code
  • TypeScript
  • Add a changeset (with link to its Feature Discussion if it exists)
  • End to end tests
  • TSDocs
  • Open an issue for this feature in the docs repo

Unrelated tasks:

  • TypeScript tests
  • Update starter themes
  • Update other packages
  • Update community discussions

@changeset-bot
Copy link

changeset-bot bot commented Jun 30, 2020

💥 No Changeset

Latest commit: 260ed0d

Merging this PR will not cause any packages to be released. If these changes should not cause updates to packages in this repo, this is fine 🙂

If these changes should be published to npm, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@codecov
Copy link

codecov bot commented Jun 30, 2020

Codecov Report

Merging #509 into dev will not change coverage.
The diff coverage is n/a.

@nicholasio
Copy link
Collaborator

This might be out of the scope of this PR but it would be great to check if there's an ads.txt file in the WP domain, that would allow users to leverage plugins like https://wordpress.org/plugins/ads-txt/

@michalczaplinski michalczaplinski self-requested a review June 30, 2020 19:51
Copy link
Member

@michalczaplinski michalczaplinski left a comment

Choose a reason for hiding this comment

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

Hey @mdesignerco! Thanks for your contribution! 🙂

I think this PR would need a little bit of work to be merged still though.

Could you:

Once that's done I can add the TSDocs and open the issue in our documentation repo!

@michalczaplinski michalczaplinski self-assigned this Jun 30, 2020
@mdesignerco
Copy link
Author

i'll see thanks!

@luisherranz
Copy link
Member

Hey guys, a couple of things.

I think it makes sense to merge this, thanks @mdesignerco for the contribution 🙂

For deploys to Vercel, we also need to update our Now builder, the same way we did it here: https://github.com/frontity/now-builder/pull/23/files @mdesignerco would you mind doing a PR there as well?

@nicholasio This might be out of the scope of this PR but it would be great to check if there's an ads.txt file in the WP domain, that would allow users to leverage plugins like https://wordpress.org/plugins/ads-txt/

That's definitely the way to go. We will accomplish these in different ways, depending on the mode:

  • Embedded mode: It will just work 🎉
  • Decoupled mode: Once we have server extensibility, we can create a package @frontity/ads-txt that adds a small Koa middleware to either:
    1. Retrieve the ads.txt from WordPress.
    2. Return the content defined in the frontity.settings.js.

We don't have yet a Feature Discussion for ads.txt, I will start one with those two examples.

@luisherranz
Copy link
Member

I've started the FD: https://community.frontity.org/t/ads-txt/2244

@mdesignerco mdesignerco closed this Jul 1, 2020
@mdesignerco
Copy link
Author

Hey guys, a couple of things.

I think it makes sense to merge this, thanks @mdesignerco for the contribution 🙂

For deploys to Vercel, we also need to update our Now builder, the same way we did it here: https://github.com/frontity/now-builder/pull/23/files @mdesignerco would you mind doing a PR there as well?

@nicholasio This might be out of the scope of this PR but it would be great to check if there's an ads.txt file in the WP domain, that would allow users to leverage plugins like https://wordpress.org/plugins/ads-txt/

That's definitely the way to go. We will accomplish these in different ways, depending on the mode:

  • Embedded mode: It will just work 🎉

  • Decoupled mode: Once we have server extensibility, we can create a package @frontity/ads-txt that adds a small Koa middleware to either:

    1. Retrieve the ads.txt from WordPress.
    2. Return the content defined in the frontity.settings.js.

We don't have yet a Feature Discussion for ads.txt, I will start one with those two examples.

Sure, i'll see this PR and i'll try. =)

@luisherranz luisherranz reopened this Jul 2, 2020
@luisherranz
Copy link
Member

Thanks for the now-builder PR @mdesignerco, it's already merged and I've just released a new version 🙂

But there's no need to close this PR, don't worry. We can also merge it for the people using Frontity in a NodeJS server.

@luisherranz luisherranz marked this pull request as draft August 28, 2020 08:30
@michalczaplinski
Copy link
Member

@luisherranz Do you think we can just merge it as is until we have server extensibility (after adding the TSDoc, changeset and link in the docs repo, of course)?

@luisherranz
Copy link
Member

Yeah, I think so. For single-site projects it's still very convenient, don't you think?

@luisherranz
Copy link
Member

@SantosGuillamot let us know what we should do with this 🙂

@SantosGuillamot
Copy link
Member

It seems it would already add value to the users, so we can merge this and work on the @frontity/ads.txt package once the Server Extensibility is ready. What is missing in the Pull Request? Just the TSDocs? I would first finish the tasks that are currently a WIP.

@michalczaplinski
Copy link
Member

For single-site projects it's still very convenient, don't you think?
yup, I think so too.

@SantosGuillamot Yes, this can be useful to some users.

There are only some "bookkeeping" tasks left:

  • TSDocs
  • changeset
  • open the issue in the Docs repo

I ll take care of this when I have a moment after finishing the support for auth headers.

@luisherranz
Copy link
Member

Awesome, thanks Michal!

@mike-niemand
Copy link

Is this complete? How do we handle ads.txt in the root?

@luisherranz
Copy link
Member

Only a changeset and a unit test and e2e test, as Michal mentioned here.

If you are willing to add those, I'll gladly review and merge this 🙂

You need to open another PR, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants