-
Notifications
You must be signed in to change notification settings - Fork 9
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
feat(search): add input search #277
Conversation
feat(search): enable typeahead feat(search): add vulnerability entity feat(search): add sbom entity feat(search): add advisory entity feat(search): add package entity feat(search): add navlink
35f5dd7
to
c2a9cd7
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.
@kahboom thank you for the PR! Looks great
I added some observations I made while testing your PR. Please take a look at them
🚀 Storybook Deployed Preview: https://trustify-ui-pr-277-preview.surge.sh ✨ |
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.
@kahboom thanks for the enhancements.
When I type "a" i see some Elements like "item.decomposedPurl?.namespace" being printed in the Dropdown component (see image below). I think that is a mistake. I know packages are tricky as even V1 do not include them in the dropdown. I don't think it would be wrong also to remove it if you consider it is better.
itemId={option.id} | ||
key={option.id} | ||
description={option.description} | ||
to={option.navLink} |
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 discovered the reason of the delay while redirecting to pages. the to
element is causing a "Full page reload". This should be replaced by the native react useNavigate
or a <Link/>
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.
For me it still shows a delay with either one (a bit improved though), but I will add it and we can always open an issue if you are still seeing it happening.
client/tsconfig.json
Outdated
@@ -1,6 +1,6 @@ | |||
{ | |||
"$schema": "https://json.schemastore.org/tsconfig", | |||
"include": ["src/**/*", "config/**/*", "types/**/*"], | |||
"include": ["src/**/*", "config/**/*", "types/**/*", "setupTests.ts"], |
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 tested your PR removing this change and everything worked. Is there a reason to add this change? I mean, all tests seem to be executed currently without this change
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 was a mistake, sorry I didn't catch it in my self review, going to remove it now. Thanks!
fix: minor changes
bb12ad4
to
0435a90
Compare
@carlosthe19916 thanks for the review!
Oops, this is how it should look now:
I've decided to keep them because we decided with Michelle to just show everything and we narrow stuff later on. Let me know how you feel about the changes. |
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.
enhancements
This PR implements #273 by adding the input search dropdown/typeahead menu for the Search Page
Changes
SearchMenu
component with local state + PF search inputScreenshots