-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Stateful sidenav] Add yml setting to force spaces solution view #191743
[Stateful sidenav] Add yml setting to force spaces solution view #191743
Conversation
/ci |
/ci |
/ci |
Pinging @elastic/appex-sharedux (Team:SharedUX) |
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.
Rule Management LGTM
ACK: reviewing... |
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.
Code LGTM, thanks! I've left a few nits and notes. The major ones are adding tests for the new property in our public contract and nesting the temporary config property under the experimental
namespace to be on the safe side when we drop the new config value.
Please, re-tag me for review once PR is ready for the final review round and I'll do it today.
Also can you please file an issue to drop this internal-only config property when we no longer need it (IIUC it should be marked as blocker for 8.17)?
Thanks for the review @azasypkin, I addressed your feedback. Can you have another look? thanks! I filled the issue to remove the yml setting. |
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.
LGTM, thanks!
|
||
expect(spacesSetup.hasOnlyDefaultSpace).toBe(false); | ||
expect(spacesStart.hasOnlyDefaultSpace).toBe(false); | ||
{ |
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.
note: { ... }
- Great way to scope and reuse const
values in the same test - never thought of this before. Nice!
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Page load bundle
History
To update your PR or re-run it, just comment with: cc @sebelga |
…stic#191743) (cherry picked from commit 6f6883d)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
#191743) (#193282) # Backport This will backport the following commits from `main` to `8.x`: - [[Stateful sidenav] Add yml setting to force spaces solution view (#191743)](#191743) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Sébastien Loix","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-09-18T12:12:20Z","message":"[Stateful sidenav] Add yml setting to force spaces solution view (#191743)","sha":"6f6883d7818a983e642a536d8892a0072a794148","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","Team:SharedUX","backport:prev-minor"],"title":"[Stateful sidenav] Add yml setting to force spaces solution view","number":191743,"url":"https://github.com/elastic/kibana/pull/191743","mergeCommit":{"message":"[Stateful sidenav] Add yml setting to force spaces solution view (#191743)","sha":"6f6883d7818a983e642a536d8892a0072a794148"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/191743","number":191743,"mergeCommit":{"message":"[Stateful sidenav] Add yml setting to force spaces solution view (#191743)","sha":"6f6883d7818a983e642a536d8892a0072a794148"}}]}] BACKPORT--> Co-authored-by: Sébastien Loix <[email protected]>
In this PR I've added an internal
kibana.yml
setting to be able to force the spaces solution view for non cloud instances. The spaces solution view enables the new solution navigations.This config is a temporary solution for Elastic internal deployments until the
8.17-SNAPSHOT
is available.New setting
Notes on implementation
We had previously twice the same logic to decide if the solution view (and side nav) feature was on: in the
navigation
andspaces
plugins. With the newyml
setting available in the spaces plugin I set the source of truth to be in the spaces plugin and the navigation plugin reads it from the start contract.Fixes #191254