-
Notifications
You must be signed in to change notification settings - Fork 894
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
[Workspace] Add global search bar into left nav #8538
Conversation
❌ Empty Changelog SectionThe Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section. |
❌ Empty Changelog SectionThe Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8538 +/- ##
==========================================
- Coverage 60.93% 60.91% -0.03%
==========================================
Files 3777 3786 +9
Lines 89852 90125 +273
Branches 14086 14131 +45
==========================================
+ Hits 54750 54897 +147
- Misses 31668 31788 +120
- Partials 3434 3440 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
❌ Entry Too LongEntry is 107 characters long, which is 7 characters longer than the maximum allowed length of 100 characters. Please revise your entry to be within the maximum length. |
Signed-off-by: Hailong Cui <[email protected]> search bar on left nav Signed-off-by: Hailong Cui <[email protected]> compressed input Signed-off-by: Hailong Cui <[email protected]>
Signed-off-by: Hailong Cui <[email protected]>
Signed-off-by: Hailong Cui <[email protected]>
Signed-off-by: Hailong Cui <[email protected]> add more unit test Signed-off-by: Hailong Cui <[email protected]>
Signed-off-by: Hailong Cui <[email protected]>
* @param value search query | ||
* @param callback callback function when search is done | ||
*/ | ||
doSearch(value: string, callback?: () => void): Promise<ReactNode[]>; |
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.
How about run
. This works well with the term command too. So it will be command.run(props)
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.
rename this method name to run
as well
So here's what i had in mind for that project (And we can impliment a subset of that today for the sake of time)
Plugins can only register commands. Pages and saved objects are used by default but we dont expose this This is very similar to how VS code does it and its really elagant. cc: @kgcreative If we dont want to go that far right now, lets keep the search bar simple and not expose any API's. This is not something we need to rush and will not be easy to walk back on in the future :) |
Signed-off-by: Hailong Cui <[email protected]>
Signed-off-by: Hailong Cui <[email protected]>
Thanks for sharing your thought on this,
|
Signed-off-by: Hailong Cui <[email protected]>
@Hailong-am From the screen record you share, it seems the highlighted text color is black and the rest are blue, while the blue text looks more like highlighted text to me, is this by design? |
That's OUI default highlight behavior and didn't provide a way to customized it. Confirmed with @kgcreative we don't do customization to the default OUI behavior. Reference: https://oui.opensearch.org/1.16/#/utilities/highlight-and-mark |
Signed-off-by: Hailong Cui <[email protected]>
src/core/public/chrome/ui/header/collapsible_nav_group_enabled.scss
Outdated
Show resolved
Hide resolved
export const SAVED_OBJECTS_SYMBOL = '@'; | ||
export const COMMANDS_SYMBOL = '>'; |
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.
It looks a little bit weird to me. Why don't we just aggregate all the results from globalSearchCommands
?
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.
That's to filter what kind of searchCommand we want to run. If user input is start with @
like @flights
it will only search for saved objects title has flights.
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.
Oh, that's a big vision in the future but seems weird that we implement in this PR. I think these should be done in an enhancement PR if we really have the use case. And we should come up with a prefix registration mechanism if that is really critical.
Anyway, thanks for doing these changes. The search functionality looks super fancy and we even comply the dev tools modal with the search commands.
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.
Agree these should come with future enhancement PR, these are built in symbols for saved objects and commands, so i put it here in advance.
regarding to prefix registration, that's might be long term plan if we plan to support more command types.
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.
Looks amazing to me and finally we get a global search function in left nav.
iconType="search" | ||
color="text" | ||
buttonRef={buttonRef} | ||
data-test-subj="search-icon" |
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.
Nit: could have a more specific test id just in case not collide with others
export const SearchCommandFilters = { | ||
[SearchObjectTypes.PAGES]: (value: string) => { | ||
return { | ||
match: !value.startsWith(SAVED_OBJECTS_SYMBOL) && !value.startsWith(COMMANDS_SYMBOL), | ||
searchValue: value, | ||
}; | ||
}, | ||
[SearchObjectTypes.SAVED_OBJECTS]: (value: string) => { | ||
return { | ||
match: value.startsWith(SAVED_OBJECTS_SYMBOL), | ||
searchValue: value.replace(SAVED_OBJECTS_SYMBOL, '').trim(), | ||
}; | ||
}, | ||
}; |
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 for discussion, instead of having these hard coded rules in one place, perhaps we can make it standardized,
// Command interface
{
id: string;
name: string;
alias?: string;
...
}
For example, the command for saved object search:
{
id: 'savedObjects.open',
name: 'asset',
alias: '@',
}
Then we can do filter based on the current input, if the input starts with asset:
or @:
, then we only show that matched command.
If the input not start with any command name/alias, the it should call run
for all commands with the current input. Each command can decide if it should run the command itself, for example, if savedObjects.open
wants to only run when the input starts with asset:
or @:
, the run function could be:
run(value: string) {
if (!value.startsWith('asset:') || !value.startsWith('@:')) return;
// ...
}
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.
That's make sense, to let each command decide what's the behavior. But for builtin pages and saved objects search command, we don't want to provide the flexibility to customized the alias, name and behavior of whether to run the command or not.
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.
If the input not start with any command name/alias, the it should call run for all commands with the current input.
I think we should search pages only, to search other types it will need explicit type the alias
<EuiFlexItem> | ||
<EuiTitle size="s"> | ||
<EuiText size="xs" color="subdued"> | ||
{i18n.translate('core.globalSearch.pageSection.title', { defaultMessage: 'Pages' })} |
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 again just for discussion.
It seems we need a specific pagesSection
based on the type
just to render the title, can we make it so that it's part of the command interface? Like:
{
id: 'workspacePagesSearchCommand',
category: 'Pages',
}
Then for all the items returns by this command, we display it under the Pages
title. Seems then we don't need type: SearchObjectTypes;
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.
Yeah, the section render could be a general component/method, the only difference is the section header. Refactor it to accept a section header props to make it reusable.
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 combine category into type and change type to a object like this
export const SearchCommandTypes = {
PAGES: {
description: i18n.translate('core.globalSearch.pages.description', { defaultMessage: 'Pages' }),
alias: null,
},
SAVED_OBJECTS: {
description: i18n.translate('core.globalSearch.assets.description', {
defaultMessage: 'Assets',
}),
alias: SAVED_OBJECTS_SYMBOL,
},
} as const;
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 header_search_bar should better not maintain page
or save_objects
, because in the future, there might be alerts
and anomaly_detectors
, these are registered by plugins, which core should NOT be aware of, so the description
and alias
should be better registered by the plugin. What do you think?
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.
Let plugin to register it makes more sense. we could add description
and alias
to GlobalSearchCommand
interface that will take precedence. And SearchCommandTypes
serve as the builtin for pages and assets.
const [devToolTab, setDevToolTab] = useState(''); | ||
|
||
const createOpenDevToolAction = createAction<typeof DEVTOOL_OPEN_ACTION>({ |
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 sharing my thought, instead of implementing it with Action/Trigger, I'd simply use a global state for the modalVisible
and devToolTab
, perhaps just a BahaviorSubject
.
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 sharing my thought, instead of implementing it with Action/Trigger, I'd simply use a global state for the
modalVisible
anddevToolTab
, perhaps just aBahaviorSubject
.
sounds like Observable is more lightweight, haven't got a chance to try it.
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.
By comparing uiActions and Observable, they are both able to handle events. the uiActions is a central place to manage events and could have context object associate with. It's more powerful and extensible. I would prefer keep the current implementation.
src/plugins/workspace/public/components/global_search/search_pages_command.tsx
Outdated
Show resolved
Hide resolved
src/plugins/workspace/public/components/global_search/search_pages_command.tsx
Show resolved
Hide resolved
Signed-off-by: Hailong Cui <[email protected]>
Signed-off-by: Hailong Cui <[email protected]>
Signed-off-by: Hailong Cui <[email protected]>
* search bar on left nav Signed-off-by: Hailong Cui <[email protected]> search bar on left nav Signed-off-by: Hailong Cui <[email protected]> compressed input Signed-off-by: Hailong Cui <[email protected]> * support search dev tools Signed-off-by: Hailong Cui <[email protected]> * enable devtools search only if new home is enabled Signed-off-by: Hailong Cui <[email protected]> * add unit test Signed-off-by: Hailong Cui <[email protected]> add more unit test Signed-off-by: Hailong Cui <[email protected]> * Changeset file for PR #8538 created/updated * fix incorrect variable name Signed-off-by: Hailong Cui <[email protected]> * dismiss tooltip when close popover Signed-off-by: Hailong Cui <[email protected]> * style adjustment Signed-off-by: Hailong Cui <[email protected]> * address review comments Signed-off-by: Hailong Cui <[email protected]> * rename search stategy to searchHandler Signed-off-by: Hailong Cui <[email protected]> * rename to searchCommand Signed-off-by: Hailong Cui <[email protected]> * exclude parent pages Signed-off-by: Hailong Cui <[email protected]> * remove duplicate return Signed-off-by: Hailong Cui <[email protected]> * address review comments Signed-off-by: Hailong Cui <[email protected]> * wording change for strategy Signed-off-by: Hailong Cui <[email protected]> * add icon for data adminstration & settings Signed-off-by: Hailong Cui <[email protected]> --------- Signed-off-by: Hailong Cui <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> (cherry picked from commit 479a3fc) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* search bar on left nav search bar on left nav compressed input * support search dev tools * enable devtools search only if new home is enabled * add unit test add more unit test * Changeset file for PR #8538 created/updated * fix incorrect variable name * dismiss tooltip when close popover * style adjustment * address review comments * rename search stategy to searchHandler * rename to searchCommand * exclude parent pages * remove duplicate return * address review comments * wording change for strategy * add icon for data adminstration & settings --------- (cherry picked from commit 479a3fc) Signed-off-by: Hailong Cui <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
) * search bar on left nav Signed-off-by: Hailong Cui <[email protected]> search bar on left nav Signed-off-by: Hailong Cui <[email protected]> compressed input Signed-off-by: Hailong Cui <[email protected]> * support search dev tools Signed-off-by: Hailong Cui <[email protected]> * enable devtools search only if new home is enabled Signed-off-by: Hailong Cui <[email protected]> * add unit test Signed-off-by: Hailong Cui <[email protected]> add more unit test Signed-off-by: Hailong Cui <[email protected]> * Changeset file for PR opensearch-project#8538 created/updated * fix incorrect variable name Signed-off-by: Hailong Cui <[email protected]> * dismiss tooltip when close popover Signed-off-by: Hailong Cui <[email protected]> * style adjustment Signed-off-by: Hailong Cui <[email protected]> * address review comments Signed-off-by: Hailong Cui <[email protected]> * rename search stategy to searchHandler Signed-off-by: Hailong Cui <[email protected]> * rename to searchCommand Signed-off-by: Hailong Cui <[email protected]> * exclude parent pages Signed-off-by: Hailong Cui <[email protected]> * remove duplicate return Signed-off-by: Hailong Cui <[email protected]> * address review comments Signed-off-by: Hailong Cui <[email protected]> * wording change for strategy Signed-off-by: Hailong Cui <[email protected]> * add icon for data adminstration & settings Signed-off-by: Hailong Cui <[email protected]> --------- Signed-off-by: Hailong Cui <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
) * search bar on left nav Signed-off-by: Hailong Cui <[email protected]> search bar on left nav Signed-off-by: Hailong Cui <[email protected]> compressed input Signed-off-by: Hailong Cui <[email protected]> * support search dev tools Signed-off-by: Hailong Cui <[email protected]> * enable devtools search only if new home is enabled Signed-off-by: Hailong Cui <[email protected]> * add unit test Signed-off-by: Hailong Cui <[email protected]> add more unit test Signed-off-by: Hailong Cui <[email protected]> * Changeset file for PR opensearch-project#8538 created/updated * fix incorrect variable name Signed-off-by: Hailong Cui <[email protected]> * dismiss tooltip when close popover Signed-off-by: Hailong Cui <[email protected]> * style adjustment Signed-off-by: Hailong Cui <[email protected]> * address review comments Signed-off-by: Hailong Cui <[email protected]> * rename search stategy to searchHandler Signed-off-by: Hailong Cui <[email protected]> * rename to searchCommand Signed-off-by: Hailong Cui <[email protected]> * exclude parent pages Signed-off-by: Hailong Cui <[email protected]> * remove duplicate return Signed-off-by: Hailong Cui <[email protected]> * address review comments Signed-off-by: Hailong Cui <[email protected]> * wording change for strategy Signed-off-by: Hailong Cui <[email protected]> * add icon for data adminstration & settings Signed-off-by: Hailong Cui <[email protected]> --------- Signed-off-by: Hailong Cui <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
Description
This PR is add search bar into left nav, the search bar supports search pages in and out of workspace.
saved objects is not supported yet.
globalsearch.mov
Issues Resolved
part of #1854
and #4055
Screenshot
Testing the changes
Changelog
Check List
yarn test:jest
yarn test:jest_integration