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

[Enterprise Search] Expose core.chrome.setIsVisible for use in Workplace Search #95984

Conversation

scottybollinger
Copy link
Contributor

@scottybollinger scottybollinger commented Mar 31, 2021

Summary

In Workplace Search, the Private dashboard has a requirement to hide the Kibana chrome, as non-admin users should not have access to anything in Kibana but their personal dashboard. To achieve this, we need to remove the chrome for non-organization routes. This PR uses KibanaLogic to expose core.chrome.setIsVisible to the app.

Because the place where this is triggered––the index file with the routes––is instantiated after the app chrome, there is an unwanted flash of the top bars before hiding the UI. We hide the chrome when mounting the Workplace Search app and then unhide it when the routes necessitate the chrome.

Although not in this PR (follow-up immediately following this one), this is what we are trying to achieve:
woot

Note: Astute observers might realize that this also hides the chrome for admin users who visit their Personal dashboard. This is an expected UX intended to keep the UXes separate for Kibana/admin management and Private source administration.

Best to review with whitespace changes hidden and/or commit-by-commit.

Checklist

The Workplace Search Personal dashboard needs the chrome hidden. We hide it globally here first to prevent a flash of chrome on the Personal dashboard and unhide it for admin routes, which will be in a future commit
@scottybollinger scottybollinger added Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes v7.13.0 auto-backport Deprecated - use backport:version if exact versions are needed labels Mar 31, 2021
@scottybollinger scottybollinger requested review from a team March 31, 2021 20:22
Comment on lines +117 to +118
// The Workplace Search Personal dashboard needs the chrome hidden. We hide it globally
// here first to prevent a flash of chrome on the Personal dashboard and unhide it for admin routes.
Copy link
Member

Choose a reason for hiding this comment

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

💯 This comment is great/adds tons of context!

Copy link
Member

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

I have some minor suggestions for tests/performance but they're non-blocking and can be done in a follow-up PR if you prefer!

@scottybollinger
Copy link
Contributor Author

@elasticmachine merge upstream

@scottybollinger
Copy link
Contributor Author

@elasticmachine merge upstream

@scottybollinger scottybollinger enabled auto-merge (squash) April 5, 2021 14:27
Copy link
Contributor

@yakhinvadim yakhinvadim left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for making this change, Scotty, and thanks for valuable suggestions, Constance!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
enterpriseSearch 1.9MB 1.9MB +268.0B

Page load bundle

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

id before after diff
enterpriseSearch 14.3KB 14.3KB +24.0B

History

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

@scottybollinger scottybollinger merged commit ea03eb1 into elastic:master Apr 5, 2021
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Apr 5, 2021
…ace Search (elastic#95984)

* Hide chrome for Workplace Search by default

The Workplace Search Personal dashboard needs the chrome hidden. We hide it globally here first to prevent a flash of chrome on the Personal dashboard and unhide it for admin routes, which will be in a future commit

* Add core.chrome.setIsVisible to KibanaLogic

* Toggle chrome visibility for Workplace Search

* Add test

* Refactor to set context and chrome when pathname changes

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine
Copy link
Contributor

💚 Backport successful

7.x / #96221

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Apr 5, 2021
…ace Search (#95984) (#96221)

* Hide chrome for Workplace Search by default

The Workplace Search Personal dashboard needs the chrome hidden. We hide it globally here first to prevent a flash of chrome on the Personal dashboard and unhide it for admin routes, which will be in a future commit

* Add core.chrome.setIsVisible to KibanaLogic

* Toggle chrome visibility for Workplace Search

* Add test

* Refactor to set context and chrome when pathname changes

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Scotty Bollinger <scotty.bollinger@elastic.co>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Apr 6, 2021
…-nav

* 'master' of github.com:elastic/kibana: (106 commits)
  [Lens] don't use eui variables for zindex (elastic#96117)
  Remove /src/legacy (elastic#95510)
  skip flaky suite (elastic#95899)
  [Dashboard] Fix Lens and TSVB chart tooltip positioning relative to global headers (elastic#94247)
  fixes a skipped management x-pack test (elastic#96178)
  [App Search] API logs: Add log detail flyout (elastic#96162)
  [tech-debt] Remove defunct opacity parameters from EUI shadow functions (elastic#96191)
  Add Input Controls project configuration (elastic#96238)
  [file upload] document file upload privileges and provide actionable UI when failures occur (elastic#95883)
  Revert "TS Incremental build exclude test files (elastic#95610)" (elastic#96223)
  [App Search] Added Sample Response section to Result Settings (elastic#95971)
  [Maps] Safe-erase text-field (elastic#94873)
  [RAC][Alert Triage][TGrid] Update the Alerts Table (TGrid) API to implement `renderCellValue` (elastic#96098)
  [Maps] Enable all zoom levels for all users (elastic#96093)
  Use plugin version in its publicPath (elastic#95945)
  [Enterprise Search] Expose core.chrome.setIsVisible for use in Workplace Search (elastic#95984)
  [Workplace Search] Add sub nav and fix rendering bugs in Personal dashboard (elastic#96100)
  [OBS]home page is showing incorrect value of APM throughput (tpm) (elastic#95991)
  [Observability] Exploratory View initial skeleton (elastic#94426)
  [KQL] Fixed styles of KQL textarea for the K8 theme (elastic#96190)
  ...

# Conflicts:
#	x-pack/plugins/snapshot_restore/__jest__/client_integration/helpers/restore_snapshot.helpers.ts
@scottybollinger scottybollinger deleted the scottybollinger/chrome-visibility branch June 24, 2021 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes v7.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants