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

Creation of documentation on how to implement js-search with gatsby #11030

Closed
wants to merge 8 commits into from
Closed

Creation of documentation on how to implement js-search with gatsby #11030

wants to merge 8 commits into from

Conversation

jonniebigodes
Copy link

Description

This pull request creates a first draft on the documentation of client side search with js-search and gatsby. Also an accompanying example is provided, and the change to the side bar file to reflect the changes.

cc @shannonbux @DSchau
Both are cced to this, one for the technical part and the other for the documentation aspect side of things if you both don't mind.

Hoping for some feedback on your part.

Related Issues

jonniebigodes added 3 commits January 4, 2019 02:11
@jonniebigodes jonniebigodes requested a review from a team January 13, 2019 02:44
@jonniebigodes jonniebigodes requested a review from a team as a code owner January 13, 2019 02:44
@LekoArts
Copy link
Contributor

Hi, thanks for the work on that! I didn't go through the doc itself yet but have a comment on your example: IMO you should completely remove the semantic styling part. It's pretty heavy (https://bundlephobia.com/result?p=semantic-ui-react@0.84.0) and looking at live preview I'd say you can style that with just plain CSS.
This will enable more people to use this example / it's more approachable as they don't have to mess with Semantic. If you have a look at the other examples: They're not high-polished pretty but functional.

@LekoArts LekoArts added the type: documentation An issue or pull request for improving or updating Gatsby's documentation label Jan 21, 2019
@jonniebigodes
Copy link
Author

I left the semantic ui part, just for styling purposes, reading it now after some time it sounds a bit overboard. With that, it's being removed and the document and example updated as we speak. Thanks for the feedback

@jonniebigodes
Copy link
Author

Any feedback on this? Is there anything that needs to be changed?

@jonniebigodes
Copy link
Author

conflicts that are popping up are due to this and has i did not want to intrude in someone else work, i left as is. The files were linted. Some feedback would be appreciated

@jonniebigodes
Copy link
Author

Closing this pull request and removing the branch, as it seems there's no intention on merging this or anything else.....If someone more capable is will to pick it up, have fun

@jonniebigodes jonniebigodes deleted the docs/client-search branch January 30, 2019 16:01
@jonniebigodes jonniebigodes restored the docs/client-search branch January 31, 2019 03:19
@jonniebigodes jonniebigodes reopened this Jan 31, 2019
Copy link
Contributor

@m-allanson m-allanson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @jonniebigodes this is a great start (also thanks for your patience on the review side of things) 👍

I ran through the tutorial and have added some comments. Having thought about it a bit, I wonder if removing some features from the example site would make the tutorial clearer. At the moment it feels like a mix between demonstrating js-search features and using js-search with Gatsby.

I think that focussing down on the using js-search with Gatsby side will make it easier to understand the example and follow along with the tutorial.

Once someone has run through the tutorial they'd be in a good spot to hop over to the js-search docs which covers specific js-search features really nicely.

For example, removing the title, author and remove stop words checkboxes, along with removing the strategy and sanitiser select boxes would make the code search implementation a fair bit shorter. What do you think?

docs/docs/adding-search-with-js-search.md Show resolved Hide resolved
Inside that folder, create a new Gatsby website using a starter template, using the command below:

```bash
npx gatsby new jsSearchExample https://github.com/gatsbyjs/gatsby-starter-default
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice use of npx :) what do you think about changing the directory name to all lowercase? No big deal either way but I think that will more closely match other guides in the site.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not propose a compromise, instead of all lowercase how about js-search-example, reason being because someone, let's call it the better half, while taking a peak at the document, she said and i quote, "Change the folder name to a hyfenated name, because and you know me i'm not a tech expert, if i was to pickup on this i would like the folder name to be as humanly readable as possible, even not being a native english speaking person" and relationship aside, she's got a point in there. 😉

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jonniebigodes that sounds great! I think we should generally use dash-case (often called kebab-case), so yeah, good call!

After all of this is done the actual implementation can be started.

Both approaches documented here are fairly generalistic so that most of the options offered by the library can be experimented with.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be a good spot to add a couple of lines about the two approaches, describing when either approach might be more appropriate.

You've got this info at the end of the second approach section already 👍, moving it up here means people could choose an approach before reading through the whole guide.

})
}
/**
* es6 fat arrow function to instantiate the search engine
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you could make these comments clearer by removing the es6 fat arrow function section and adding a note to the Prerequisites section that you'll need some familiarity with es6 syntax like arrow functions.


The approach documented below is a fairly simple one, by having the component fetch the data and create the search engine.

Start by creating a file in the `components` folder, for this particular case the name will be `SearchContainer.js` and inside of it the following baseline code will be added to get started:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add src/ to the folder name and re-arrange to make a little shorter?

Suggested change
Start by creating a file in the `components` folder, for this particular case the name will be `SearchContainer.js` and inside of it the following baseline code will be added to get started:
Start by creating a file named `SearchContainer.js` in the `src/components/` folder, then add the following code to get started:

"gatsby-plugin-manifest": "^2.0.9",
"gatsby-plugin-offline": "^2.0.16",
"gatsby-plugin-react-helmet": "^3.0.2",
"gatsby-plugin-sharp": "^2.0.14",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about basing this example on gatsby-starter-hello-world instead of gatsby-starter-default? The hello world starter has less dependencies and a smaller gatsby-config.js file, which would make it easier to see which parts are related to setting up js-search.


Note:

For brevity purposes, most of the component implementation is omited, with the exception of the `searchData()` function, as this one is responsible for making the search, also the implementation of the `DataTable` component and the page holding this component is not shown. The full code of this example is available [in the js-search repo](https://github.com/gatsbyjs/gatsby/tree/master/examples/using-js-search).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how you've highlighted the relevant parts of the code in the tutorial here. But I can see this part being confusing for someone that's followed along so far, I got a bit stuck working out which files I needed to copy from the example.

An approach that could make this clearer is to reduce the amount of files used in the example site. So you could do something like this:

  • delete search.css and apply those styles inline in the DataTable.js component
  • Move the code from DataTable.js into SearchContainer.js, and then delete the DataTable.js file

Which might not be strictly best practice, but we can add comments explaining how you'd do it in a "real" site, and it'll make the follow up instructions shorter.

Doing this means that someone following this tutorial only needs to copy components/SearchContainer.js and then edit pages/index.js to get their version into a working state.

Then the instructions here can clearly describe which files should be copied from the example site to your local site. After making the previous changes, this could be something like To get this running in your site, copy the SearchContainer.js file to your site, and then update the file pages/index.js [as shown here](#)

Edit: I added some more thoughts about tightening up this section in the overall PR comment.

export default ClientSearch
```

The code used in here is almost identical to the one documented in the first approach and for the same reason stated in the first approach, here also the full implementation will not be shown.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to my earlier comment, let's make it very clear how to get from this step of the tutorial to a working local site:

  • combine files if possible
  • clearly describe which files should be copied to your local site
  • mention starting the site with gatsby develop and the url to visit

Copy link
Contributor

@shannonbux shannonbux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One requested change (change to "you" pronoun") and one optional change (headers)


## Setup

Let's start by creating a new Gatsby site to work with. Open up a terminal and create the folder that you'll use for this project:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part of the Gatsby Style Guide is to use "you" as the pronoun in the docs. Here's an explanation of how to make this change and why! https://www.gatsbyjs.org/docs/gatsby-style-guide/#use-you-as-the-pronoun


For brevity purposes, most of the component implementation is omited, with the exception of the `searchData()` function, as this one is responsible for making the search, also the implementation of the `DataTable` component and the page holding this component is not shown. The full code of this example is available [in the js-search repo](https://github.com/gatsbyjs/gatsby/tree/master/examples/using-js-search).

## Second approach
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if there's a more precise headers than "first approach" and "second approach". Because sometimes people share URLs with people and the URL will look more readable the headers are specific.

@jonniebigodes
Copy link
Author

By the time you'll be reading this. This branch will be closed and deleted, this due to the fact the probably #10772 and closing and re-opening the pull request introduced some issues with my fork. While trying to commit the updated document and the example, i'm presented with the following:

git_push_error

Tried updating my fork, issued git pull and retried, same error. So to avoid "throwing the kitchen sink" (pardon the bad pun). It would be best to start with a clean slate and go from there.
The other pull request i have, namelly #11233 will stay open for now as i'm addressing the changes proposed by @marcysutton .
@shannonbux thank you for the input, is being taken under advisement, i'm currently trying out a couple of synonyms with the document so that it can be further refined

@jonniebigodes jonniebigodes deleted the docs/client-search branch February 1, 2019 23:44
@m-allanson
Copy link
Contributor

Note: this work was continued in #11505

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: documentation An issue or pull request for improving or updating Gatsby's documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants