-
Notifications
You must be signed in to change notification settings - Fork 1k
Conversation
Please find a preview at: https://staging.nodejs.dev/1677/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functionality-wise, it looks good to me.
We'll have to deal with these console errors though
One other thing I noticed is that the primary js file in the production bundle increased from 27.7KB to 52.6 KB, maybe because for client-side search it must have been bundling the data about all the learn pages everywhere.
If that's the case, this size is likely to increase a lot more when we start indexing docs and blogs, and that might be a concern.
nodejs.dev
This PR
Good point... I didn't look at the bundle sizes. I did see an option to lazy load in the docs for https://www.gatsbyjs.com/plugins/gatsby-plugin-local-search/ but I get a cross-origin react error when I try to use it. |
df1f89b
to
a5e9be1
Compare
Please find a preview at: https://staging.nodejs.dev/1677/ |
Please find a preview at: https://staging.nodejs.dev/1677/ |
749cff6
to
d0e769f
Compare
Please find a preview at: https://staging.nodejs.dev/1677/ |
367d7a0
to
b9e8299
Compare
Codecov Report
@@ Coverage Diff @@
## main #1677 +/- ##
==========================================
- Coverage 90.67% 88.88% -1.79%
==========================================
Files 81 83 +2
Lines 879 927 +48
Branches 241 262 +21
==========================================
+ Hits 797 824 +27
- Misses 81 102 +21
Partials 1 1
Continue to review full report at Codecov.
|
Please find a preview at: https://staging.nodejs.dev/1677/ |
6761d80
to
44b45be
Compare
Please find a preview at: https://staging.nodejs.dev/1677/ |
19e9d6f
to
19fb363
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
quick demo for a client side search
Move all the state and function of SearchBar to SearchInput. Let only the query in SearchBar,so it only pass the result of the query as a props to SearchInput.
Add filter to the query for learn pages. So, the search result will be only pages that are related to the learn category.
19fb363
to
67d2a66
Compare
67d2a66
to
182c7c8
Compare
This reverts nodejs/nodejs.dev#1677
Initial implementation of a client side search.
It current only searches through the /learn material.
The search bar is only visible on /learn or /learn/* at the moment.
Things todo in future PR's
co-author: @Mpaulo95