Skip to content
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

[Serverless] New UX for Global Search bar #158978

Merged
merged 8 commits into from
Jun 21, 2023
Merged

[Serverless] New UX for Global Search bar #158978

merged 8 commits into from
Jun 21, 2023

Conversation

tsullivan
Copy link
Member

@tsullivan tsullivan commented Jun 2, 2023 •
edited
Loading

Global Search

Project document: https://docs.google.com/document/d/1Z-AkRoagS6JMVN5YHZpjgQr0sYGFXBTNS9SHsbLPfGQ/edit#

Summary

Closes #159763

Currently there is a known issue with the search bar when there is not enough screen real estate: #154415

Checklist

Delete any items that are not applicable to this PR.

@tsullivan tsullivan added the Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) label Jun 2, 2023
@tsullivan tsullivan self-assigned this Jun 2, 2023
@tsullivan tsullivan force-pushed the serverless/add-global-search-ux branch 2 times, most recently from afcebfd to 2e8bd57 Compare June 2, 2023 22:56
@tsullivan tsullivan added the Project:Serverless Work as part of the Serverless project for its initial release label Jun 2, 2023
@tsullivan tsullivan force-pushed the serverless/add-global-search-ux branch 3 times, most recently from ffb2c0f to 5ccf620 Compare June 6, 2023 23:31
@tsullivan tsullivan changed the title Global Search component for serverless projects [POC/Services] Global Search service changes Jun 7, 2023
@tsullivan tsullivan force-pushed the serverless/add-global-search-ux branch 2 times, most recently from 4f2eca6 to d9f70ad Compare June 9, 2023 00:18
@tsullivan tsullivan mentioned this pull request Jun 9, 2023
Merged
2 tasks
@tsullivan tsullivan force-pushed the serverless/add-global-search-ux branch 6 times, most recently from f625eb6 to 9e40881 Compare June 15, 2023 01:30
@tsullivan tsullivan changed the title [POC/Services] Global Search service changes [Serverless] New UX for Global Search bar Jun 15, 2023
@tsullivan tsullivan force-pushed the serverless/add-global-search-ux branch from 2724026 to 1c5ea66 Compare June 15, 2023 01:40
@tsullivan tsullivan force-pushed the serverless/add-global-search-ux branch from 11a3972 to 8418f07 Compare June 15, 2023 22:45
@tsullivan tsullivan marked this pull request as ready for review June 15, 2023 22:46
@tsullivan tsullivan requested a review from a team as a code owner June 15, 2023 22:46
@elasticmachine
Copy link
Contributor

Pinging @elastic/appex-sharedux (Team:SharedUX)

@tsullivan tsullivan added the release_note:skip Skip the PR/issue when compiling release notes label Jun 15, 2023
Dosant
Dosant approved these changes Jun 20, 2023
View reviewed changes
Copy link
Contributor

@Dosant Dosant left a comment

Choose a reason for hiding this comment

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

Lgtm,

Noticed this issue that on some screen sizes close button is not visible. Probably need to adjust the input to be more flexible. It is hardcoded width: 400px;

Screenshot 2023-06-20 at 13 47 25

x-pack/plugins/global_search_bar/public/components/search_bar.tsx Outdated
if (chromeStyle === 'project' && input) {
// while the input ref is in flight, we set focus on it
// to autofocus the input when it appears
input.focus();
Copy link
Contributor

Choose a reason for hiding this comment

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

Would the autoFocus prop work here?

searchProps={{
        autoFocus: chromeStyle === 'project',

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, this will be much better than my hack 🤦🏻

Copy link
Member Author

Choose a reason for hiding this comment

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

f4678a1

x-pack/plugins/global_search_bar/public/components/search_bar.tsx Outdated
const chromeStyle = useObservable(chromeStyle$);

// These hooks are used when on chromeStyle set to 'project'
const [isVisible, setIsVisible] = useLocalStorage(GLOBAL_SEARCH_BAR_VISIBLE_KEY, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

I found this a bit weird that we keep this state inside the storage. It seems that usually, this kind of state is just in memory.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think there is benefit to preserving the selection, since the search bar used to be always visible and now it takes a click to show it. If it's ok, I'll go ahead with it this way for now, and bring it up for discussion in the project document.

Copy link
Member Author

@tsullivan tsullivan Jun 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

You might be right that it's better to not store this. I would prefer to make this a simple PR and not add anything that isn't really necessary. If there is friction, we can store the state with a tiny change.

Pushed 82c5e60

@tsullivan
Copy link
Member Author

@elasticmachine merge upstream

@tsullivan tsullivan enabled auto-merge (squash) June 21, 2023 17:34
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
globalSearchBar 31 33 +2

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
globalSearchBar 21.3KB 22.4KB +1.1KB
Unknown metric groups

ESLint disabled line counts

id before after diff
enterpriseSearch 13 15 +2
securitySolution 416 420 +4
total +6

Total ESLint disabled count

id before after diff
enterpriseSearch 14 16 +2
securitySolution 497 501 +4
total +6

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @tsullivan

@tsullivan tsullivan merged commit ec3b60b into elastic:main Jun 21, 2023
@kibanamachine kibanamachine added v8.9.0 backport:skip This commit does not require backporting labels Jun 21, 2023
@tsullivan tsullivan deleted the serverless/add-global-search-ux branch November 21, 2023 23:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Dosant Dosant Dosant approved these changes

Assignees

@tsullivan tsullivan

Labels
backport:skip This commit does not require backporting Project:Serverless Work as part of the Serverless project for its initial release release_note:skip Skip the PR/issue when compiling release notes Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) v8.9.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement new project-style UX for Global Search
5 participants
@tsullivan @elasticmachine @kibana-ci @Dosant @kibanamachine