-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
Gatsby library search page #3456
Conversation
Deploy preview for gatsbygram ready! Built with commit cb316ca |
Looking good! https://deploy-preview-3456--gatsbyjs.netlify.com/plugins/ One quick bit of feedback — we use Yarn to manage dependencies not NPM so please remove the package-lock.json files that were added. |
Awesome to see this on #3003! Thanks @jastack! Some next steps:
Random design stuff:
|
www/src/components/searchbar-body.js
Outdated
|
||
import typography from "../utils/typography" | ||
|
||
function Search(){ |
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.
const Search = () =>
www/src/components/searchbar-body.js
Outdated
style={{ | ||
color: `white`, | ||
border: `0`, | ||
boxShadow: `0 0 0 0`, |
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.
boxShadow: none
should work
www/src/components/searchbar-body.js
Outdated
</Link> | ||
<div css={{ | ||
fontFamily: typography.options.bodyFontFamily.join(`,`), | ||
fontSize: `${13 / 16 * 100}%`, |
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.
scale(n)
www/src/components/searchbar-body.js
Outdated
<div css={{ | ||
fontFamily: typography.options.bodyFontFamily.join(`,`), | ||
fontSize: `${13 / 16 * 100}%`, | ||
lineHeight: `1.1`, |
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.
rhythm(n)
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.
(theoretically a whole # or simple fraction like 3/4)
www/src/components/searchbar-body.js
Outdated
fontFamily: typography.options.bodyFontFamily.join(`,`), | ||
fontSize: `${13 / 16 * 100}%`, | ||
lineHeight: `1.1`, | ||
paddingTop: `7px`, |
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.
rhythm(n)
(theoretically a whole # or simple fraction like 3/4)
www/src/html.js
Outdated
<link | ||
rel="stylesheet" | ||
href="https://unpkg.com/[email protected]/style.min.css" | ||
/> |
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.
do we need / use this?
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.
I do use their styling for the search box, but mostly included it so I could have something that looked decent fairly quickly. Can definitely take this out and do our own.
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.
Got it. Didn't realize we were using it. Let's talk about styling in one fell swoop, but for now just add rel="preload"
www/src/components/searchbar-body.js
Outdated
borderBottom: `1px solid grey`, | ||
}}> | ||
<div css={{ | ||
padding: `10px`, |
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.
use rhythm
www/src/components/searchbar-body.js
Outdated
) | ||
} | ||
|
||
function Result({ hit }){ |
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.
const Result = ({ hit }) => {
Is there a compelling reason to change the existing URL structure? |
On pulling in all plugins — we should include in the search any package on NPM with the |
@KyleAMathews could be a bit weird if the landing page is /plugins but links go to /packages/x |
another thought on routing -- since we're not searching strictly plugins, we're searching all packages, let's have this live at /packages (we could make /plugins redirect to /packages too if we want). |
re docsearch & algolia -- two things: (1) The medium-term plan is to, at build time, have a source plugin that uses Algolia npm-search (https://blog.algolia.com/yarn-search-javascript-packages/) to pull all package details including the readme into Gatsby data, build a page for each package. (2) In the short run, this is a great start and probably no need to fiddle further with algolia stuffbefore merging |
Thanks guys! I'm going to work on some fixes based on your feedback then get back to you |
@jastack This is exciting! I helped with the first prototypes for this and interviewed a bunch of Gatsby users to figure out what features are essential, so it's cool to see it progressing!! A couple notes:
|
💯 to this
yeah, let's use Gatsby Library |
Also please do not forget "search by Algolia" @jastack #3003 (comment) Probably want to link "Algolia" to either like: The main website …is probably safest bet since I think we plan to include both doc search and npm-search. @KyleAMathews? @calcsam? thoughts? Should we loop Haroen in to help decide? |
…to text in packages
Alright, finally pushed another draft! Check it out at "/packages" now. @calcsam and @KyleAMathews - the biggest change is that now the search pulls from Algolia's npm-search, instead of from only source packages, which means any package with the "gatsby-plugin" tag shows up. I realize this means more work because as Sam mentioned, we will need to have a source plugin in that pulls in all the other package details to build a page for each. So if it makes sense to stick with source package search in the short-term we can do that. @shannonbux let me know what you think about the styling. I included more info on each search result, like tags, last updated, and downloads and can add icons too. |
Let's just do the source plugin for Two tweaks to the search
Really starting to come together! Excited to see this happen! |
www/src/pages/packages.js
Outdated
textAlign: `center`, | ||
}} | ||
|
||
>Welcome to the Gatsby Plugin and Starter Library!</h1> |
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.
Let's change "plugin and starter" to just "package" since we're not going to search for starters here
www/src/components/searchbar-body.js
Outdated
justifyContent: `center`, | ||
}}> | ||
<SearchBox | ||
translations={{placeholder: 'Search Gatsby Library by keyword'}} |
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.
Should be Search Gatsby Library
@@ -0,0 +1 @@ | |||
|
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.
prob want to remove this file :)
www/src/components/withUrlSync.js
Outdated
|
||
const updateAfter = 700; | ||
const searchStateToQueryString = searchState => ({ | ||
q: searchState.query, |
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.
no using one-letter variables outside of math class :) query
and page
are more understandable
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.
ignore if not using file
www/src/components/withUrlSync.js
Outdated
...(searchState.refinementList.keywords && { | ||
keywords: searchState.refinementList.keywords, | ||
}), | ||
}), |
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.
can you break this out into a couple of lines with descriptive variable names? it would be more understandable that way.
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.
ignore if not using file
www/src/components/withUrlSync.js
Outdated
page: p || 1, | ||
refinementList: { | ||
...(keywords && { keywords }), | ||
...(owner && { 'owner.name': owner }), |
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.
what's the reason for 'owner.name' instead of 'owner'?
is there a reason why this function isn't just:
const parsedQuery = qs.parse(queryString)
return {
...parsedQuery,
page: parsedQuery.page || 1
}
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.
ignore if not using file
www/src/components/withUrlSync.js
Outdated
this.state = { | ||
searchState: queryStringToSearchState(location.search.slice(1)), | ||
}; | ||
window.addEventListener('popstate', ({ state: searchState }) => { |
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.
event listeners should be in componentDidMount and cleaned up in componentWillUnmount
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.
ignore if not using file
www/src/components/searchbar-body.js
Outdated
onSearchStateChange = searchState => { | ||
clearTimeout(this.debouncedSetState); | ||
this.debouncedSetState = setTimeout(() => { | ||
this.props.history.replace(`/packages?=${searchState.query}`) |
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.
keep this for now, but...
...we shouldn't be manually manipulating props // the global , we should be calling navigateTo which is how gatsby handles navigation.....
....however, this may not work at the moment with search queries due to a separate bug: #2074. (We need to check for differences in location.pathname and location.search) So hold off on changing this for a bit, I'll let you know when I address that and then you can use navigateTo.
www/src/html.js
Outdated
<link | ||
rel="stylesheet" | ||
href="https://unpkg.com/[email protected]/style.min.css" | ||
/> |
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.
Got it. Didn't realize we were using it. Let's talk about styling in one fell swoop, but for now just add rel="preload"
www/src/components/withUrlSync.js
Outdated
}; | ||
}; | ||
|
||
const originalPathName = location.pathname; |
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.
this is going to be evaluated outside of the component lifecycle, which is bad. try evaluating it in a constructor
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.
ignore if not using file
www/src/components/withUrlSync.js
Outdated
@@ -0,0 +1,91 @@ | |||
import React, { Component } from 'react'; |
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.
File should be capitalized 'WithUrlSync' since it's a component
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.
.......wait, are we using this file at all? I don't see it referenced anywhere
www/src/components/withUrlSync.js
Outdated
} | ||
|
||
onSearchStateChange = searchState => { | ||
clearTimeout(this.debouncedSetState); |
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.
see above stuff re debouncing
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.
ignore if not using file
…and package detail page
I made some progress but I'm trying to resolve a couple of these testing/build errors on the most current commit (cb316ca) to the branch without much luck, the tests that I see failing (4 in total) running
This looked similar to errors on #1754, and seem like they could be issues introduced by changes in the yarn.lock with updated dependencies. Does that seem possible? And the snapshots with errors (4) are little changes like this. I'm not familiar with when you'd need to update a jest snapshot but the changes I see to the code for the plugin library don't look like they'd be causing an issue like this, and it could be the result of something that was merged oddly.
I think I'll reset back to find a commit where these tests weren't failing to identify what really is causing them. Any other ideas @KyleAMathews? Maybe it's even simpler than I think. |
@gillkyle the changes being caused by an odd merge could definitely be an issue. You may want to try a cherry-picking or interactive rebase strategy to isolate and squash the commits that are part of this branch (the ones by jastack) and rebase on top of a fresh master. |
@gillkyle tests can sometimes fail locally due to no fault of your own — Gatsby has a ton of packages and if a few of them fail to install correctly (seems like a near guarantee...) then some of tests will fail. Pay more attention to TravisCI failing then what's happening locally. None of those tests are affected by this PR so there's probably nothing that needs fixed. Resolve the conflicting files and then see if TravisCI passes tests. |
I was just looking into it, I'll merge it in and try building it on my branch with Travis CI, thanks for the help! |
Just chatted about this with @shannonbux a little and remembered that I had drawn a bunch of icons for the mobile navigation in case "Plugins" became its own section: The two above the "magic wand" icon a) pick up the quite common "puzzle pieces" metaphor for "Plugins", b) show data flowing through those plugins (and/or "Transformers"), all within the Gatsby monogram/"G" shape. Not super clear, so probably not a way to go. Anyways…questions:
Adding another item to the main navigation is another nudge to re-think the latter in terms of the home page layout… ;-) For now I think the additional "Library" should fit in with that landing page from ~900px browser width. Since I'd hate to lose the other navigation items when going below that width, maybe https://css-tricks.com/the-priority-navigation-pattern/ is one way to proceed long-term? Or we could revamp the homepage layout… :-( |
Will starters be indexed/part of the library at some point too? |
@fk I have no attachment to any particular name at this point; plugins is
probably clearer!
And about starters--we considered combining them with plugins, then decided
against it because we wanted a more visual showcase that in my imagination,
could also be a way to share templates. Open to discussion on where to put
an index of starters.
…On Thu, Feb 22, 2018 at 2:28 PM, Florian Kissling ***@***.***> wrote:
Will starters be indexed/part of the library at some point too?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3456 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/Ae9o2q1-aYBJ5S1TiljFcstnT4duZgabks5tXdvpgaJpZM4RYm-i>
.
|
yeah, like Shannon said. The Library is for plugins and Gatsby-specific React components. |
Though let's close this PR since work is continuing over on @gillkyle's PR |
Added a draft of the plugin search with Algolia. Check it out by going to "/plugins/". The left side search bar functions in a very similar way to the regular sidebar, so I made some changes to the template index.js as well. Let me know if this is a good approach / going in the right direction! Doesn't have filter right now, but should be easy to add. @calcsam