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.

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

[Serverless Chrome] Polish of home logo and project switcher #158523

Merged

Conversation

tsullivan
Copy link
Member

@tsullivan tsullivan commented May 25, 2023

Closes #157810
Closes #158879

Summary

  1. Moves the Logo icon out of the side nav and to the left of the breadcrumbs
  2. Moves the project switcher from the right of the header to the just to the right of the logo
  3. Removes the link to cloud from the side nav. Design is still TBD.
  4. Adds new serverless.setProjectHome API since the home link is no longer in the side nav
  5. Removes the linkToCloud prop from the Navigation component since design is still TBD.
  6. Exposes the Global Search bar

Checklist

Delete any items that are not applicable to this PR.

Screenshots

Default
Screenshot 2023-06-06 at 11 46 52 AM
Global search (not yet collapsed by default)
Screenshot 2023-06-06 at 11 46 37 AM
Collapsed side nav
image

@tsullivan tsullivan force-pushed the shared-ux/chrome/navigation-polish-ii branch 2 times, most recently from 393890f to 34a6336 Compare May 27, 2023 03:48
@tsullivan tsullivan marked this pull request as ready for review May 27, 2023 03:48
@tsullivan tsullivan requested a review from a team as a code owner May 27, 2023 03:48
@tsullivan tsullivan requested a review from a team May 27, 2023 03:48
@tsullivan tsullivan added the Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) label May 27, 2023
@tsullivan tsullivan self-assigned this May 27, 2023
@elasticmachine
Copy link
Contributor

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

@tsullivan tsullivan added Project:Serverless Work as part of the Serverless project for its initial release release_note:skip Skip the PR/issue when compiling release notes labels May 27, 2023
@tsullivan tsullivan force-pushed the shared-ux/chrome/navigation-polish-ii branch from 34a6336 to ae61074 Compare May 28, 2023 22:56
@sebelga
Copy link
Contributor

sebelga commented May 30, 2023

I wonder if we can wait for the "v2" UI to be merged (#158297) before this one as most of the changes will have re-done.

@tsullivan tsullivan requested a review from a team as a code owner June 2, 2023 22:15
@tsullivan tsullivan marked this pull request as draft June 2, 2023 22:15
@tsullivan tsullivan force-pushed the shared-ux/chrome/navigation-polish-ii branch 3 times, most recently from 6c54ecc to 6727a4a Compare June 6, 2023 18:54
@tsullivan tsullivan force-pushed the shared-ux/chrome/navigation-polish-ii branch from 6727a4a to 2e904ff Compare June 6, 2023 19:08
@@ -9,7 +9,7 @@
import React from 'react';
import { FormattedMessage } from '@kbn/i18n-react';
import { BehaviorSubject, combineLatest, merge, type Observable, of, ReplaySubject } from 'rxjs';
import { flatMap, map, takeUntil } from 'rxjs/operators';
import { mergeMap, map, takeUntil } from 'rxjs/operators';
Copy link
Member Author

Choose a reason for hiding this comment

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

flatMap is deprecated

@@ -177,25 +177,23 @@ export class ChromeService {
chromeStyle$.next(style);
};

const setProjectSideNavComponent = (component: ISideNavComponent | null) => {
const validateChromeStyle = () => {
const chromeStyle = chromeStyle$.getValue();
Copy link
Member Author

Choose a reason for hiding this comment

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

changed the validation into a helper function

},
nav: {
toggleNavButton: css`
border-right: 1px solid #d3dae6;
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if there is a cleaner way to do this in CSS. It may be better to leave the header style alone for now since changes will be coming to this area in an EUI update.

cc @cee-chen

Copy link
Contributor

Choose a reason for hiding this comment

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

You'll need to use the useEuiTheme hook to get the JS theme variables for border width/colors. But yeah I'll be working on EuiCollapsibleNav updates next week here, which will include this styling, so if you want you could leave this out!

Copy link
Member Author

@tsullivan tsullivan Jun 7, 2023

Choose a reason for hiding this comment

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

@cee-chen thanks for the updates! I'm tempted to update this PR to use useEuiTheme and ensure the border for this header item (the nav show/hide button) is the right color, even if temporarily.

This discussion probably belongs here: elastic/eui#6759 (comment)

<EuiHeaderSection side="right">
<EuiHeaderSectionItem>
<HeaderNavControls navControls$={observables.navControlsCenter$} />
<HeaderNavControls navControls$={observables.navControlsRight$} />
Copy link
Member Author

@tsullivan tsullivan Jun 6, 2023

Choose a reason for hiding this comment

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

The global search bar is in the center slot, and the help menu is in the right.
image

{/* TODO: This puts a group of nav menu items on the right edge of the screen,
but it should be possible for apps customize the layout in a grid and use spacers between items.
https://github.com/elastic/kibana/issues/158034 */}
<EuiHeaderSection />
Copy link
Member Author

Choose a reason for hiding this comment

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

removed this comment since we won't focus on more work here in the short-term, and the design may change once we get to it.

@@ -53,22 +53,23 @@ export class ServerlessPlugin
if (developer && developer.projectSwitcher && developer.projectSwitcher.enabled) {
const { currentType } = developer.projectSwitcher;

core.chrome.navControls.registerRight({
core.chrome.navControls.registerLeft({
Copy link
Member Author

Choose a reason for hiding this comment

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

moves the project switcher to a left-side slot of the header

Copy link
Contributor

@gbamparop gbamparop left a comment

Choose a reason for hiding this comment

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

Observability changes LGTM!

Copy link
Member

@sphilipse sphilipse left a comment

Choose a reason for hiding this comment

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

ent-search changes LGTM

Copy link
Contributor

@semd semd left a comment

Choose a reason for hiding this comment

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

Security Solution LGTM!

@tsullivan
Copy link
Member Author

I received feedback from @clintandrewhall the "project switcher" component was mistakenly moved in this PR. I'm working on resolving the fix and will hold off on merging for now.

Copy link
Contributor

@clintandrewhall clintandrewhall left a comment

Choose a reason for hiding this comment

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

Great work. Just resolve your conflicts, and you should be g2g. Thanks!

@tsullivan tsullivan enabled auto-merge (squash) June 14, 2023 01:24
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
serverlessObservability 45 35 -10
serverlessSearch 89 81 -8
total -18

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/shared-ux-chrome-navigation 44 41 -3
serverless 13 15 +2
total -1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
serverlessSearch 51.9KB 55.3KB +3.4KB

Page load bundle

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

id before after diff
core 367.7KB 369.3KB +1.7KB
serverless 5.2KB 5.3KB +110.0B
serverlessObservability 25.2KB 16.5KB -8.7KB
serverlessSearch 28.5KB 19.8KB -8.7KB
serverlessSecurity 68.3KB 68.4KB +34.0B
total -15.6KB
Unknown metric groups

API count

id before after diff
@kbn/core-chrome-browser 150 149 -1
@kbn/shared-ux-chrome-navigation 68 62 -6
serverless 14 16 +2
total -5

ESLint disabled line counts

id before after diff
enterpriseSearch 13 15 +2
securitySolution 409 413 +4
total +6

Total ESLint disabled count

id before after diff
enterpriseSearch 14 16 +2
securitySolution 492 496 +4
total +6

History

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

cc @tsullivan

@tsullivan tsullivan merged commit cabfebe into elastic:main Jun 14, 2023
@kibanamachine kibanamachine added v8.9.0 backport:skip This commit does not require backporting labels Jun 14, 2023
tsullivan added a commit that referenced this pull request Aug 11, 2023
## Summary

This PR adjusts the `data-test-subj` for the global loading indicator in
serverless projects such that at matches the stateful version. This
makes sure that functional tests and corresponding test helper methods
continue to work the same in stateful and serverless environments when
comes to waiting for global loading to finish, which is a key mechanism
to avoid test flakiness.

### Additional information

- The serverless project specific global loading indicator was
introduced with #158523
- The stateful loading indicator `data-test-subj` naming is implemented
here:
https://github.com/elastic/kibana/blob/main/packages/core/chrome/core-chrome-browser-internal/src/ui/loading_indicator.tsx#L61

Co-authored-by: Tim Sullivan <[email protected]>
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Aug 11, 2023
…163697)

## Summary

This PR adjusts the `data-test-subj` for the global loading indicator in
serverless projects such that at matches the stateful version. This
makes sure that functional tests and corresponding test helper methods
continue to work the same in stateful and serverless environments when
comes to waiting for global loading to finish, which is a key mechanism
to avoid test flakiness.

### Additional information

- The serverless project specific global loading indicator was
introduced with elastic#158523
- The stateful loading indicator `data-test-subj` naming is implemented
here:
https://github.com/elastic/kibana/blob/main/packages/core/chrome/core-chrome-browser-internal/src/ui/loading_indicator.tsx#L61

Co-authored-by: Tim Sullivan <[email protected]>
(cherry picked from commit fd08c62)
kibanamachine added a commit that referenced this pull request Aug 11, 2023
…63697) (#163745)

# Backport

This will backport the following commits from `main` to `8.9`:
- [Adjust global loading indicator data-test-subj for projects
(#163697)](#163697)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Robert
Oskamp","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-08-11T17:40:26Z","message":"Adjust
global loading indicator data-test-subj for projects (#163697)\n\n##
Summary\r\n\r\nThis PR adjusts the `data-test-subj` for the global
loading indicator in\r\nserverless projects such that at matches the
stateful version. This\r\nmakes sure that functional tests and
corresponding test helper methods\r\ncontinue to work the same in
stateful and serverless environments when\r\ncomes to waiting for global
loading to finish, which is a key mechanism\r\nto avoid test
flakiness.\r\n\r\n### Additional information\r\n\r\n- The serverless
project specific global loading indicator was\r\nintroduced with
#158523\r\n- The stateful loading indicator `data-test-subj` naming is
implemented\r\nhere:\r\nhttps://github.com/elastic/kibana/blob/main/packages/core/chrome/core-chrome-browser-internal/src/ui/loading_indicator.tsx#L61\r\n\r\nCo-authored-by:
Tim Sullivan
<[email protected]>","sha":"fd08c62f052d065e677b670a381840ae80dc9724","branchLabelMapping":{"^v8.10.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","backport:prev-minor","v8.10.0","v8.9.1"],"number":163697,"url":"https://github.com/elastic/kibana/pull/163697","mergeCommit":{"message":"Adjust
global loading indicator data-test-subj for projects (#163697)\n\n##
Summary\r\n\r\nThis PR adjusts the `data-test-subj` for the global
loading indicator in\r\nserverless projects such that at matches the
stateful version. This\r\nmakes sure that functional tests and
corresponding test helper methods\r\ncontinue to work the same in
stateful and serverless environments when\r\ncomes to waiting for global
loading to finish, which is a key mechanism\r\nto avoid test
flakiness.\r\n\r\n### Additional information\r\n\r\n- The serverless
project specific global loading indicator was\r\nintroduced with
#158523\r\n- The stateful loading indicator `data-test-subj` naming is
implemented\r\nhere:\r\nhttps://github.com/elastic/kibana/blob/main/packages/core/chrome/core-chrome-browser-internal/src/ui/loading_indicator.tsx#L61\r\n\r\nCo-authored-by:
Tim Sullivan
<[email protected]>","sha":"fd08c62f052d065e677b670a381840ae80dc9724"}},"sourceBranch":"main","suggestedTargetBranches":["8.9"],"targetPullRequestStates":[{"branch":"main","label":"v8.10.0","labelRegex":"^v8.10.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/163697","number":163697,"mergeCommit":{"message":"Adjust
global loading indicator data-test-subj for projects (#163697)\n\n##
Summary\r\n\r\nThis PR adjusts the `data-test-subj` for the global
loading indicator in\r\nserverless projects such that at matches the
stateful version. This\r\nmakes sure that functional tests and
corresponding test helper methods\r\ncontinue to work the same in
stateful and serverless environments when\r\ncomes to waiting for global
loading to finish, which is a key mechanism\r\nto avoid test
flakiness.\r\n\r\n### Additional information\r\n\r\n- The serverless
project specific global loading indicator was\r\nintroduced with
#158523\r\n- The stateful loading indicator `data-test-subj` naming is
implemented\r\nhere:\r\nhttps://github.com/elastic/kibana/blob/main/packages/core/chrome/core-chrome-browser-internal/src/ui/loading_indicator.tsx#L61\r\n\r\nCo-authored-by:
Tim Sullivan
<[email protected]>","sha":"fd08c62f052d065e677b670a381840ae80dc9724"}},{"branch":"8.9","label":"v8.9.1","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Robert Oskamp <[email protected]>
@tsullivan tsullivan deleted the shared-ux/chrome/navigation-polish-ii branch November 21, 2023 23:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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