8000 Fix search in docs (#254) by rstegg · Pull Request #255 · evilsoft/crocks · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix search in docs (#254) #255

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Apr 6, 2018
Merged

Fix search in docs (#254) #255

merged 5 commits into from
Apr 6, 2018

Conversation

rstegg
Copy link
Contributor
@rstegg rstegg commented Apr 5, 2018 •
edited
Loading

This change only uses Title, Description, and Tags as the search fields, and adds function names to the tags field of their relative documentation.

@coveralls
Copy link
coveralls commented Apr 5, 2018 •
edited
Loading

Coverage Status

Coverage remained the same at 100.0% when pulling 5540544 on rstegg:rsteg-docs-search into ac363e9 on evilsoft:master.

@evilsoft
Copy link
Owner
evilsoft commented Apr 5, 2018 •
edited
Loading

Is there a way to get function names into the the search, like with assoc as was mentioned by @HenriqueLimas in this issue.

Also it would be great to add some description on how this fixes the problem in the PR comment. Helps with knowledge sharing for people browsing the PRs.

@evilsoft
Copy link
Owner
evilsoft commented Apr 5, 2018 •
edited
Loading

I would recommend adding a functions tag to the meta data.
Then on the pages that contain definitions for function, add a list of their names...
like for combinators this should be:

functions: "applyTo composeB constant flip identity substitution"

and for maybe this should be:

functions: "find prop propPath safe safeAfter safeLift eitherToMaybe firstToMaybe lastToMaybe resultToMaybe"

Notice for the ADT, only for functions that are defined in that module, not the methods.

EDIT: Then we just add the function field to the list of fields to search against.

@rstegg
Copy link
Contributor Author
rstegg commented Apr 5, 2018

This is fixed with tags now.

HenriqueLimas
HenriqueLimas reviewed Apr 5, 2018
View reviewed changes
Copy link
Collaborator
@HenriqueLimas HenriqueLimas left a comment •
edited
Loading

Choose a reason for hiding this comment

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

That is amazing, I was to do that and you were fastest 😄 👏
Just one thing do you know if is it possible to change the result link on each tag? Like for instance on assoc I wanted to link directly go to the assoc definition /functions/helpers.html#assoc

docs/src/pages/docs/functions/transformation-functions.md Outdated
weight: 60
---


Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure we need that extra spaces

@rstegg
Copy link
Contributor Author
rstegg commented Apr 5, 2018 •
edited
Loading

It doesn't seem like that is possible with the current search engine supplied. Though, that would be nice to have.

Here's the parameters available: https://github.com/electricjs/electric/blob/master/packages/electric-base-components/src/ElectricSearchBase.js#L131

@evilsoft
Copy link
Owner
evilsoft commented Apr 5, 2018

I would say that this is a good step forward.
We should get this into the pipeline as it will at least get us to the page.

Now I am wondering @HenriqueLimas if you know of a hook that builds out the search reference data and if we could hook it where we are building out the md hooks for header links here. Curious if there is some way in that config to add "custom" entries to the dataset used to search on.

But unless anyone says Boo, I say we merge this up and get it into the pipeline.
We can iterate one we have a clean means to insert custom recs into the query data.

@HenriqueLimas
Copy link
Collaborator

Agree with @evilsoft, I think we can merge it. This will improve a lot the search. I will take a look if that is possible and if not, I will talk with guys (add an issue) on electric.js

Nice work @rstegg

@evilsoft evilsoft self-requested a review April 6, 2018 03:55
evilsoft
evilsoft approved these changes Apr 6, 2018
View reviewed changes
Copy link
Owner
@evilsoft evilsoft left a comment

Choose a reason for hiding this comment

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

This looks good. Gonna ship this and explore ways to further enhance the search capabilities with additional iteration.

@evilsoft
Copy link
Owner
evilsoft commented Apr 6, 2018

Have a Fix It Squirrel
image

@evilsoft evilsoft merged commit 309c9d9 into evilsoft:master Apr 6, 2018
@evilsoft evilsoft mentioned this pull request Apr 6, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HenriqueLimas HenriqueLimas HenriqueLimas left review comments

@evilsoft evilsoft evilsoft approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rstegg @coveralls @evilsoft @HenriqueLimas
0