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(default-theme): native support for carbon ads #86

Merged
merged 9 commits into from
Nov 26, 2020

Conversation

posva
Copy link
Member

@posva posva commented Sep 14, 2020

I also added the existing slots sidebar-top/bottom and page-top/bottom because I used them to add the ads components. To make them work and test them, you need to add this config to the themeConfig:

    carbonAds: {
      carbon: 'CEBICK3I',
      custom: 'CEBICK3M',
      placement: 'routervuejsorg',
    },

@kiaking
Copy link
Member

kiaking commented Oct 12, 2020

I kinda like the idea of having Google Analytics option, since usually everybody wants that. But not sure about the ads... Because while Vue's official docs uses Carbon, it really depends on locales what the primary ads formats are. I think for now, ads should be configured for each repo (Router/Vuex/etc.) using slots 🤔 What do you think? (Again, I'm all for Google Analytics though).

@kiaking kiaking added the enhancement New feature or request label Oct 12, 2020
@posva
Copy link
Member Author

posva commented Oct 15, 2020

I think we should still add the slots to the default theme and then add configs for ads to the vue theme since we use it in every official vue lib documentation. We could also handle something per locale, and it could also be a plugin instead.

Adding analytics should also be in a plugin like in vuepress.

But I don't think we have the plugin architecture yet

@kiaking
Copy link
Member

kiaking commented Oct 19, 2020

Ah, that make sense. If we were to implement plugins feature (and I think we kinda need it), it make sense now to include these and pull it out once we have the plugin feature 👍

@posva
Copy link
Member Author

posva commented Oct 19, 2020

yeah, let's hold on to this for the moment. I will refactor it once there is some kind of plugin architecture in place

@kiaking
Copy link
Member

kiaking commented Nov 5, 2020

@posva I think at the moment, VitePress doesn't look like to have any strong plugin feature implemented in future. I think we should have this merged in to the core directly.

@posva
Copy link
Member Author

posva commented Nov 5, 2020

@kiaking I think it's fine to wait for the moment! Better have a plugin architecture built that allows adding such things.
Do you want to add this feature for Vuex or something? The current PR is not up to date with what exists in vue router next either. I would have to update it. I even plan on changing what I currently have on Vue Router docs to allocate spots for sponsors.

@kiaking
Copy link
Member

kiaking commented Nov 5, 2020

I think it's fine to wait for the moment! Better have a plugin architecture built that allows adding such things.

No I mean the discussion we are having right now, there might be no plugin feature for VitePress at all. So we might be waiting for nothing... 🤔

Do you want to add this feature for Vuex or something?

This is another story but yes I would love to. And for that, I can of course wait!

@posva
Copy link
Member Author

posva commented Nov 20, 2020

I will give this a go and refactor next week and make it look like the current version on vue router next docs: https://next.router.vuejs.org/guide/essentials/navigation.html#navigate-to-a-different-location which was inspired by vuejs.org

@kiaking
Copy link
Member

kiaking commented Nov 20, 2020

Yeah great idea! Looking forward to it! I might be modifying the components a lot for stylings and refactoring. So don't worry so much about conflicts, I think I can resolve those.

@posva
Copy link
Member Author

posva commented Nov 23, 2020

Here are the changes needed. The overflow-x: auto fixes this:

Screen Shot 2020-11-23 at 12 19 29

into

Screen Shot 2020-11-23 at 12 43 52

Note you will have to add that style rule to .scrimba for vuex docs

@posva posva changed the title Add native support for carbon ads and buy sell ads feat(default-theme): native support for carbon ads Nov 23, 2020
@posva
Copy link
Member Author

posva commented Nov 23, 2020

I think we should add by default the algolia search too, but I will do that in a separat PR. What do you think @kiaking ?

@kiaking
Copy link
Member

kiaking commented Nov 23, 2020

Yes we should definitely have algolia integration too, the issue #40 🙌

@posva
Copy link
Member Author

posva commented Nov 23, 2020

ah yeah, that's a different thing though 😆 : simple search out of the box based on titles. Algolia still needs you to request ids at https://docsearch.algolia.com/

@posva
Copy link
Member Author

posva commented Nov 26, 2020

This should be ready for review!

@kiaking
Copy link
Member

kiaking commented Nov 26, 2020

Here we go!

@kiaking kiaking merged commit 9d6b8ca into vuejs:master Nov 26, 2020
@posva posva deleted the feat/carbondads branch November 26, 2020 14:45
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants