-
Notifications
You must be signed in to change notification settings - Fork 44
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
DOCS-876: Use instantsearch and typesense for tutorials page #1693
Conversation
bea9c54
to
8b929d8
Compare
a84b4a7
to
e7ddf3c
Compare
layouts/partials/sidebar-tree.html
Outdated
@@ -51,6 +51,8 @@ | |||
{{ $manualLink := cond (isset $s.Params "manuallink") $s.Params.manualLink ( cond (isset $s.Params "manuallinkrelref") (relref $s $s.Params.manualLinkRelref) $s.RelPermalink) -}} | |||
{{ $manualLinkTitle := cond (isset $s.Params "manuallinktitle") $s.Params.manualLinkTitle $s.Title -}} | |||
{{ $empty_node := $s.Params.empty_node -}} | |||
|
|||
{{ warnf "%s %s %s" $s $s.RelPermalink $s.Permalink }} |
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.
{{ warnf "%s %s %s" $s $s.RelPermalink $s.Permalink }} |
b53c69c
to
c322353
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
@@ -16,8 +16,8 @@ authors: [ "Hazal Mestci" ] | |||
languages: [ "python" ] | |||
viamresources: [ "base", "vision", "camera" ] | |||
level: "Beginner" | |||
date: "18 August 2022" | |||
updated: "11 August 2023" | |||
date: "2022-08-18" |
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.
(opt) I don't want to block bc I don't think it matters too much but I will say american ppl might get confused by this date format
date: "2022-08-18" | |
date: "08-18-2022" |
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 think it's not just american people, but in this case this is how Go expects the date to be formatted, so it is what it is
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 no issues -- a couple comments with suggestions in case you want to implement
cost: "0" | ||
no_list: true | ||
featured: 4 |
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 how much we've featured it on socials?
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 an optional value that determines the sorting. So this one should appear in 4th position. I could change this to weight to be more in line with how we think about this in the rest of the docs?
@@ -15,13 +15,16 @@ tags: ["tutorial"] | |||
draft: true # Change this when you're ready | |||
authors: [] # Your Name |
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.
Would add "featured" to template here
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.
done
cacheSearchResultsForSeconds: 2 * 60, // Cache search results from server. Defaults to 2 minutes. Set to 0 to disable caching. | ||
}, | ||
// The following parameters are directly passed to Typesense's search API endpoint. | ||
// So you can pass any parameters supported by the search endpoint below. |
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 seems useful but I would suggest having more info here if this tip will be important for understanding search functionality in the future - is this endpoint what people are using/querying in the search bar of docs? and code here is for that?
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.
the search bar works similar but with a different api key and different parameters. It's two different collections that the data for searching is stored in. This code dynamically adds the search sidebar and the tutorial cards to the page by accessing the collection and searching for tutorials that fit the search criteria and then adding them to the page
assets/scss/_styles_project.scss
Outdated
width: 100%; | ||
} | ||
|
||
.ais-Pagination-item { |
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.
(opt) If you know how to toggle button size on square buttons, it seems like the buttons might be smaller than they appear.
I don't know if this is the right object (here where I'm commenting) but I did notice at least in staging a lot of the buttons in the tutorials index/nav bar don't work for me if I try to click them, at first, they only have a small surface area. Tiny issue and non blocking, but if you could make the buttons slightly bigger it would be completely unnoticeable.
Only the buttons that look like <<
and >>
are easily clickable, the rest I have to hover for a bit. Not sure how to fix, but might be a browser related issue on my end?
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.
Just some high-level comments, nothing blocking, LGTM.
- Replacing left-nav is a cool conservation of real estate, but may confuse users who are exploring the site, and now cannot return to the rest of it. If I want to look at tutorials to get a feel of available options, but then want to look at services to do same, I cannot unless I explicitly select a (any) tutorial to restore the left-nav (or know to click icon in upper-left). This will likely confuse.
- Having checkboxes disappear on the left, instead of grey-out, looks like a browser problem or that something is broken. You mean: checkboxes removed do not have a valid union with current selection(s). They see: I clicked one thing and the other got lost, and I don't remember what it said to know if I wanted it or not. Greyed-out makes this clear: not compatible with current selection(s).
- Approx cost should include the currency. Just a
$
before the first - or both - text inputs should be fine!
|
@npentrel Top would work fine, and I think make sense! You are already capping presented tuts at 12 per page, so I don't think an options bar at top would be too much for content pane -- but restoring user-expected left-nav functionality outweighs in my opinion. Thanks for considering! |
18d3557
to
e1ae3c3
Compare
You can view a rendered version of the docs from this PR at https://docs-test.viam.dev/3a2daa4191862927e16084282e0e85e89163dd8d/public |
https://docs-test.viam.dev/21dbe0b2507868d3e2054a9a9639ae22a7751641/public/tutorials/