Disable Search Box if CDN is down (#9) #186
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
|
Deploy preview ready! Built with commit 1d1656c |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
| @@ -13,6 +13,7 @@ import React from 'react'; | |||
| import {colors, fonts, media} from 'theme'; | |||
| import {version} from 'site-constants'; | |||
| import ExternalLinkSvg from 'templates/components/ExternalLinkSvg'; | |||
| import DocSearch from 'components/DocSearch'; | |||
There was a problem hiding this comment.
Organizational nit: This new DocSearch component can just live within the src/components/LayoutHeader folder since it's only referenced there.
Particularly since there are styles defined on the DocSearch component (eg flex: '0 0 auto') that are dependent on its parent.
| componentDidMount() { | ||
| // Initialize Algolia search. | ||
| // TODO Is this expensive? Should it be deferred until a user is about to search? | ||
| // eslint-disable-next-line no-undef |
There was a problem hiding this comment.
Might be nice but might also be unnecessary. Feel free to leave it for a follow-up, or if you're interested in maybe doing it- measure the perf first to see if it's actually worth doing?
There was a problem hiding this comment.
Maybe not, I just thought of removing all that complexity from the Template component and just let the DocSearch do everything, mostly because of its own setState behavior.
There was a problem hiding this comment.
Yeah, I think moving the initialization here makes sense. 👍
I'm not sure we need to defer the initialization until it's used though. (I actually tried it locally really quick to see if it was simple to just defer until the first "focus" event, but it didn't work without a double click and I don't see any mention to a complete-callback in Algolia's docs so... 😄
| } | ||
|
|
||
| render() { | ||
| const {disabled} = this.state; |
There was a problem hiding this comment.
I think we should just render null if search is unavailable. That's less confusing than a disabled search input I think. What do you think?
There was a problem hiding this comment.
Oh, sorry, I thought you meant to disable the input when you said "3. Render the component in a disabled state if so". 😥 😃
There was a problem hiding this comment.
No worries at all. I was originally thinking the same, but then I tried this out and thought it might be better just to not show the input at all 😁
| inputSelector: '#algolia-doc-search', | ||
| }); | ||
| } else { | ||
| this.setState({disabled: true}); |
There was a problem hiding this comment.
Might be nice to add a console.warn that search failed to load and so it's being disabled.
What's the error / stack trace you're seeing in the console after these changes have been made? |
The same from before:
|
|
Ah, I see. I thought before that you were getting an error about |
|
Turning the Cookie button "On" works for me, (which is bizarre since #14 is related to enabling it). I'm not sure if it helps, but it's good to consider that Cookie Whitelist is actually a Legacy add-on:
|
|
I'm changing the PR name to point to #9. |
SadPandaBear
changed the title
Sync with reactjs.org @ d0f2db9
In English: For example, this code will not behave as you might expect because** 0 will be printed **when props.messages is an empty array: 是 ** 0会被渲染 ** 而不是 ~~<MessageList />会被渲染~~
Should help fixing #9 and be a potentially help for #14.