Add/alogliadocs #158
Add/alogliadocs #158
Conversation
components/DocsLayout.js
Outdated
| @@ -44,6 +44,7 @@ class DocsLayout extends Component { | |||
| title={`styled-components${title ? `: ${title}` : ''}`} | |||
| description={description}> | |||
| <meta name="robots" content="noodp" /> | |||
| <link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/docsearch.js@2/dist/cdn/docsearch.min.css" /> | |||
There was a problem hiding this comment.
I'd either vendor it into our code or style it manually, I think to keep the requests low. We're already at a high number of requests due to Google
There was a problem hiding this comment.
Sure! I wanted to do that, but I thought maybe styles are huge to include. So I'll do this with injectGlobal or I should add a style tag inside <Head> by hand?
There was a problem hiding this comment.
So, the difference will be small especially on HTTP 2, but this is a small CSS file after all. I can verify what it actually contains and see.
There was a problem hiding this comment.
btw About Google, we should probably replace that with something else, maybe Heap. Google Analytics is not accurate and also it's so heavy.
There was a problem hiding this comment.
We spent months getting GA getting set up correctly, I'm not switching to anything different now. I highly, highly doubt anything else is more accurate.
There was a problem hiding this comment.
Tim data engineer at Unsplash: https://medium.com/@timmycarbone/google-analytics-modifies-your-data-24d4d6366210
it's only one of the articles that mention this in detail.
GA is also really heavy.
components/Nav/Search.js
Outdated
| /> | ||
| </Wrapper> | ||
| ) | ||
| class Search extends Component { |
There was a problem hiding this comment.
This should not be in the same file, including the logic above for the import
| @@ -5,7 +5,7 @@ import rem from '../../utils/rem' | |||
| import { navbarHeight } from '../../utils/sizes' | |||
| import Link from '../Link' | |||
|
|
|||
| const Wrapper = styled.nav` | |||
| const Wrapper = styled.div` | |||
There was a problem hiding this comment.
Yea, we already have a nav for all of the navbar AFAIK. There's no need to nest multiple navs!
…-components-website into add/alogliadocs
morajabi
dismissed
kitten’s stale review
Extracted Algolia logic from Search.js and did some refactoring. Also now search only appears on /docs
Do we not want people to search from the landing page? |
|
@mxstbr We can. I thought Algolia will not respond on the other urls, but it seems it's not true. I'll make it available everywhere and will change the placeholder to UPDATE: Thanks Max for pointing out! Done. |
|
Do we have a deploy of this somewhere? |
|
@mxstbr No... until now 😉 https://styled-components-docs-dmtxaiqtba.now.sh |
|
That looks great! 😍 I'm not seeing a search in the navbar on the docs pages though? |
|
@mxstbr That's thanks to the awesome Algoliadocs! (we can customize the styling easily though if we wanted to) Are you sure it's not cached or something? |
|
Oh yeah now it's loaded, weird. |
|
And this? :) |
components/Nav/SearchWithAlgolia.js
Outdated
| componentDidMount() { | ||
| if (process.browser && typeof docsearch !== 'undefined') { | ||
| docsearch({ | ||
| apiKey: '79886fb59ad3ebe2002b481cffbbe7cb', |
There was a problem hiding this comment.
dunno, but I suppose you can see the email they've sent me in the issue. They clearly said use it in the client side.
There was a problem hiding this comment.
Yeah if it's client-side it has to be safe to commit.
components/Nav/SearchWithAlgolia.js
Outdated
| } | ||
|
|
||
| componentDidMount() { | ||
| if (process.browser && typeof docsearch !== 'undefined') { |
There was a problem hiding this comment.
Let's require docsearch down here instead and remove the process.browser stuff since componentDidMount doesn't run on the server
| isDocs = true | ||
|
|
||
| componentWillMount() { | ||
| this.isDocs = process.browser && Router.pathname.startsWith('/docs') |
There was a problem hiding this comment.
I don't see why this should have process.browser? Isn't the router universal?
…-components-website into add/alogliadocs
|
I can't wait to land this 😍 Such an important enhancement! |
|
Ugh why haven't we shipped this yet?! @morajabi mind resolving the conflicts? Then I'll go ahead, merge and ship this. |
|
I deleted yarn.lock and regenerated it. |
|
Ugh even more conflicts. 🙄 I suck at this, mind merging master in again? That should fix the tests then we can finally ship this! |
|
Why my PRs are always like this 😄 It's all on @philpl 😈💩 |
|
Haha I'm so sorry, I should've shipped this first! 🙈 My bad! |
|
@mxstbr I'm almost always joking haha isn't it visible from tons of emojis I use?!?!?! |
|
Any help needed getting this merged? |
|
@Haroenv Thank you, Haroen, I forgot to rebase this, lemme try again. |
|
@morajabi looks like this needs a lot of rebasing |
|
@morajabi I think we should start integrating DocSearch again. I requested new keys from Algolia. Can I fix this in a different PR? |
Fix #85
I've set up the search in the navbar. We are using Algolia docsearhc and the crawler config is live here https://github.com/algolia/docsearch-configs/blob/master/configs/styled-components.json
We can make PRs there to alter it. Also, the styling is the default one and we can customize the look by following the DocSearch documentation:
(https://community.algolia.com/docsearch/documentation/) Which I tried and couldn't change it the way to be better and thought it is unnecessary at this point too, but we can do that at any point.