Skip to content

Conversation

@MoonMoon1919
Copy link
Contributor

What it is

Adds module search to Library reference landing page

@netlify
Copy link

netlify bot commented Jun 1, 2023

Deploy Preview for pensive-meitner-faaeee ready!

Name Link
🔨 Latest commit 1d17098
🔍 Latest deploy log https://app.netlify.com/sites/pensive-meitner-faaeee/deploys/64825d62f488db000839d0fd
😎 Deploy Preview https://deploy-preview-850--pensive-meitner-faaeee.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@ebeneliason
Copy link
Contributor

  • nit: the search field should have the same border radius, font size, and text colors as the other elements.
  • nit: it would be nice to use the same purple focus color as for the main docs search. Related, and perhaps making this less of a nit: it looks like there's a weird redundant blue focus on the main docs search field.
  • Ideally the search field would auto-focus when loading this page.
  • Josh suggested grouping the topics in the filter by use case (DevOps Foundations, Running apps, Storing data). If that's already supported by this component it would be nice to drop in right away.
  • If we don't yet support a "service" type, we should just hide the "type" filter for now.
  • We definitely need to show 2–3 lines of the description for this to be usable.
  • Should we rename this page to something like "Find a Module"?
  • Josh has recommended that we switch to using the canonical module names (i.e. the hyphenated names you reference in code) instead of these "friendly" phrasings, and instead incorporate the friendly phrasings into the descriptions. That's probably a separate story, but I wanted to call it out.
  • Now that we have search, should we collapse the "service catalog" and "module catalog" sections in the sidebar to reduce visual distraction when landing on this page?
  • As mentioned in our initial review, we may want to revise the appearance of these cards, but this is a great first step. I wouldn't hold up this PR (or even the story) to make those changes, but let's talk about next steps.

@MoonMoon1919
Copy link
Contributor Author

MoonMoon1919 commented Jun 6, 2023

  • nit: the search field should have the same border radius, font size, and text colors as the other elements.

Will add, thanks! Done ✅

  • nit: it would be nice to use the same purple focus color as for the main docs search. Related, and perhaps making this less of a nit: it looks like there's a weird redundant blue focus on the main docs search field.

Will add, thanks! Added ✅

fixed the redundant blue focus, thanks!

  • Ideally the search field would auto-focus when loading this page.

  • Josh suggested grouping the topics in the filter by use case (DevOps Foundations, Running apps, Storing data). If that's already supported by this component it would be nice to drop in right away.

We don't have the topic attribute in the search index yet. When we add it it should be as simple as changing the name of the facet we're retrieving.

  • If we don't yet support a "service" type, we should just hide the "type" filter for now.

  • We definitely need to show 2–3 lines of the description for this to be usable.

Working on this.

  • Should we rename this page to something like "Find a Module"?

  • Josh has recommended that we switch to using the canonical module names (i.e. the hyphenated names you reference in code) instead of these "friendly" phrasings, and instead incorporate the friendly phrasings into the descriptions. That's probably a separate story, but I wanted to call it out.

Got it, thanks!

  • Now that we have search, should we collapse the "service catalog" and "module catalog" sections in the sidebar to reduce visual distraction when landing on this page?

I'm personally a fan of the sidebar, but if we want to collapse it, I'm fine with that.

  • As mentioned in our initial review, we may want to revise the appearance of these cards, but this is a great first step. I wouldn't hold up this PR (or even the story) to make those changes, but let's talk about next steps.

Sounds good, thanks!

@ebeneliason
Copy link
Contributor

We don't have the topic attribute in the search index yet. When we add it it should be as simple as changing the name of the facet we're retrieving.

I'm confused by this. Searching by topic already appears to work. I realize we don't have "use cases" in our search index, but the idea here would just be to hard-code a grouping of our topic list into those three use case areas purely for display within the menu itself, at least for now.

I'm personally a fan of the sidebar, but if we want to collapse it, I'm fine with that.

Let's talk about this more before acting on it. Josh has actually proposed eliminating the browsable sidebar in favor of a bespoke filter sidebar (where you could e.g. check and uncheck various topics and other filter options instead of the dropdown). That's clearly not in scope here — I bring it up just to call out that we may consider the UI of this reference section more holistically at some point.

@MoonMoon1919
Copy link
Contributor Author

I'm confused by this. Searching by topic already appears to work. I realize we don't have "use cases" in our search index, but the idea here would just be to hard-code a grouping of our topic list into those three use case areas purely for display within the menu itself, at least for now.

Let's chat offline. Perhaps I'm confused as well.

@MoonMoon1919 MoonMoon1919 force-pushed the feat/CORE-867/find-module-im-looking-for branch from bf81b2b to 98dc50f Compare June 7, 2023 17:44
@MoonMoon1919 MoonMoon1919 changed the title [WIP] feat(CORE-876): Implement module search feat(CORE-876 & CORE-8665): Implement module search and display all modules Jun 8, 2023
@MoonMoon1919 MoonMoon1919 changed the title feat(CORE-876 & CORE-8665): Implement module search and display all modules feat(CORE-876 & CORE-865): Implement module search and display all modules Jun 8, 2023
export const SearchArea: React.FunctionComponent<
PropsWithChildren<SearchAreaProps>
> = ({ name, requirement, type, children }) => {
// TODO: Make using a configuration work here
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we want to leave this TODO in as a future reminder or pull out from this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

im pulling it out now, i forgot to do it before i removed the WIP

zackproser
zackproser previously approved these changes Jun 8, 2023
Copy link
Contributor

@zackproser zackproser left a comment

Choose a reason for hiding this comment

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

LGTM!

@ebeneliason
Copy link
Contributor

Looking good! I suggest shortening the intro blurb considerably. I would have made this a suggestion on the PR but the unchanged sentence in the middle made that impossible.

This section contains a complete list of the modules, services, and tools included in the Gruntwork IaC Library, along with reference documentation for each. For general information on the structure and usage of the Library, refer to our IaC Library docs. If you can't find a module or service that suits your needs, let us know at feedback@gruntwork.io.

  • Yesterday I believe I saw that we have the repo (module) name available in the index already. Without carrying that all the way through to the sidebar and reference pages, can we update the cards in this list to use that format straight away? I'm curious how that looks, and if the wrapping is weird. It would, hopefully, eliminate a few of the cards where the title wraps to several lines and breaks the layout a bit (something to fix later in itself).
  • Let's switch "All" to "Any" in the type filter.
  • It looks like the styling on the search field got lost.
  • What's up with the "OPEN VPN PACKAGE INFRASTRUCTURE PACKAGE" tags?

eak12913
eak12913 previously approved these changes Jun 8, 2023
Copy link
Contributor

@eak12913 eak12913 left a comment

Choose a reason for hiding this comment

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

LGTM!

@MoonMoon1919
Copy link
Contributor Author

Looking good! I suggest shortening the intro blurb considerably. I would have made this a suggestion on the PR but the unchanged sentence in the middle made that impossible.

  • Yesterday I believe I saw that we have the repo (module) name available in the index already. Without carrying that all the way through to the sidebar and reference pages, can we update the cards in this list to use that format straight away? I'm curious how that looks, and if the wrapping is weird. It would, hopefully, eliminate a few of the cards where the title wraps to several lines and breaks the layout a bit (something to fix later in itself).

  • Let's switch "All" to "Any" in the type filter.

  • It looks like the styling on the search field got lost.

Will ping you on this

  • What's up with the "OPEN VPN PACKAGE INFRASTRUCTURE PACKAGE" tags?

Looks like they are long enough that it's wrapping lines and the background color is continuing on across the whole line

eak12913
eak12913 previously approved these changes Jun 8, 2023
Copy link
Contributor

@eak12913 eak12913 left a comment

Choose a reason for hiding this comment

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

LGTM

eak12913
eak12913 previously approved these changes Jun 8, 2023
Copy link
Contributor

@eak12913 eak12913 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@eak12913 eak12913 left a comment

Choose a reason for hiding this comment

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

LGTM

@MoonMoon1919 MoonMoon1919 merged commit 4e173f7 into master Jun 9, 2023
@MoonMoon1919 MoonMoon1919 deleted the feat/CORE-867/find-module-im-looking-for branch June 9, 2023 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants