[Stateful sidenav] Welcome tour#194926
Conversation
|
/ci |
1 similar comment
|
/ci |
cabcc6a to
1ef3294
Compare
|
/ci |
1 similar comment
|
/ci |
|
/ci |
💔 Build Failed
Failed CI StepsTest Failures
Metrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: cc @sebelga |
4cd3226 to
c9fca06
Compare
|
/ci |
|
Pinging @elastic/appex-sharedux (Team:SharedUX) |
azasypkin
left a comment
There was a problem hiding this comment.
I have a few nits, but my main concern is that we seem to be calling the Spaces API even if we know the user has already closed the tour. This is problematic since the tour is a one-time thing for new users, while the redundant call will affect everyone.
| const showTour$ = new BehaviorSubject(showTourUiSettingValue ?? true); | ||
|
|
||
| const allSpaces$ = defer(() => from(spacesManager.getSpaces())).pipe(shareReplay(1)); |
There was a problem hiding this comment.
Imaging all the logic/variables you'd have to put in place for the cache + deal with potential concurrent calls to the Promise (which makes an HTTP request)
Well, I don't think these additional variables would be that scary comparing to BehaviorSubject, defer, pipes, switchMap, shareReplay, subscribe, and unsubscribe machinery that one should be sufficient familiar with to understand the flow.
Just for the record, I had something like that in mind (not tested though, semi-pseudo code):
export function initTour(core: CoreStart, spacesManager: SpacesManager) {
const showTourUiSettingValue = core.settings.globalClient.get(SHOW_SPACE_SOLUTION_TOUR_SETTING);
async function shouldShowTour() {
// We don't show tour if there are already multiple spaces in the cluster.
const spaces = await spacesManager.getSpaces();
if (spaces.length > 1) {
return false;
}
// We don't show tour if default space's solution is `classic`.
const defaultSpace = spaces.find(({ id }) => id === 'default');
return !defaultSpace.solution || defaultSpace?.solution === SOLUTION_VIEW_CLASSIC;
}
function hideTourInGlobalSettings() {
core.settings.globalClient.set(SHOW_SPACE_SOLUTION_TOUR_SETTING, false).catch(() => {
// Silently swallow errors, the user will just see the tour again next time they load the page
});
}
// Don't make call to Spaces API if the setting is explicitly set to false.
let cachedShouldShowTourPromise: Promise<boolean> | null =
showTourUiSettingValue === false ? Promise.resolve(false) : null;
return {
shouldShowTour: () => {
if (!cachedShouldShowTourPromise) {
cachedShouldShowTourPromise = shouldShowTour().then((shouldShow) => {
// Disable the tour if the setting isn't yet explicitly set to false.
if (!shouldShow && showTourUiSettingValue !== false) {
hideTourInGlobalSettings();
}
return shouldShow;
});
}
return cachedShouldShowTourPromise;
},
onFinishTour: () => {
hideTourInGlobalSettings();
cachedShouldShowTourPromise = Promise.resolve(false);
},
};
}Anyway, code style is not something I’d block the functionality over as long as it's performant and functionally correct. I’m just genuinely curious why people like Observables, since I don’t, at least in the vast majority of cases.
x-pack/plugins/spaces/public/nav_control/solution_view_tour/lib.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/spaces/public/nav_control/solution_view_tour/solution_view_tour.tsx
Outdated
Show resolved
Hide resolved
x-pack/test/functional/apps/spaces/solution_view_flag_enabled/solution_tour.ts
Show resolved
Hide resolved
| version = version ?? (await kibanaServer.version.get()); | ||
| version = version.replace(/-SNAPSHOT$/, ''); | ||
|
|
||
| log.debug(`Deleting [config-global:${version}] doc from the .kibana index`); | ||
|
|
||
| await es | ||
| .delete( | ||
| { id: `config-global:${version}`, index: '.kibana', refresh: true }, | ||
| { headers: { 'kbn-xsrf': 'spaces' } } | ||
| ) | ||
| .catch((error) => { | ||
| if (error.statusCode === 404) return; // ignore 404 errors | ||
| throw error; | ||
| }); | ||
| }; |
There was a problem hiding this comment.
question: @elastic/kibana-core, is a workaround like this the only way we have to “reset” config-global in tests?
x-pack/plugins/spaces/public/nav_control/solution_view_tour/lib.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/spaces/public/nav_control/solution_view_tour/solution_view_tour.tsx
Outdated
Show resolved
Hide resolved
| oblt: i18n.translate('xpack.spaces.navControl.tour.obltSolution', { | ||
| defaultMessage: 'Observability', | ||
| }), | ||
| classic: '', // Tour is not shown for the classic solution |
There was a problem hiding this comment.
It creates other types of problem.
Are you referring to the fact that we always render SolutionViewTour no matter what solution the active space is configured with?
|
Thanks for the review @azasypkin. I addressed all your feedback, can you have another look? thanks! |
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Page load bundle
History
cc @sebelga |
azasypkin
left a comment
There was a problem hiding this comment.
Okay, I think we’re good to merge. Thanks for addressing my feedback. That said, before merging, let’s also address all remaining IMPORTANT notes.
x-pack/plugins/spaces/public/nav_control/nav_control_popover.test.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/spaces/public/nav_control/solution_view_tour/lib.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/spaces/public/nav_control/solution_view_tour/lib.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/spaces/public/nav_control/solution_view_tour/lib.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/spaces/public/nav_control/solution_view_tour/lib.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/spaces/public/nav_control/solution_view_tour/solution_view_tour.tsx
Outdated
Show resolved
Hide resolved
|
|
||
| import type { SolutionView } from '../../../common'; | ||
|
|
||
| const tourLearnMoreLink = 'https://ela.st/left-nav'; |
There was a problem hiding this comment.
Important
How long do you think we’ll need to rely on this link? Who currently has permissions to edit or delete it? Is your plan to eventually move this to the proper Kibana docs and use the docLinks service instead?
I’m a bit concerned that we don’t have any guardrails to prevent this link from breaking or pointing to anything that it shouldn't point to. Unlike normal doc links changes to to this short URL doesn't require any review.
There was a problem hiding this comment.
I am guessing this tour will stay for 1 or max 2 releases. @kevinsweet controls that short URL. I think we should be able to coordinate before he deletes the link, making sure we have removed the tour in the UI.
There was a problem hiding this comment.
Thanks, we should be good to go then (wasn't sure if it was going to stay forever).
There was a problem hiding this comment.
@kevinsweet please correct my assumptions here. If they are correct maybe we could already open the issue to remove the tour in release X?
x-pack/test/functional/apps/spaces/solution_view_flag_enabled/solution_tour.ts
Outdated
Show resolved
Hide resolved
x-pack/test/functional/apps/spaces/solution_view_flag_enabled/solution_tour.ts
Outdated
Show resolved
Hide resolved
x-pack/test/functional/apps/spaces/solution_view_flag_enabled/solution_tour.ts
Show resolved
Hide resolved
…bana into stateful-sidenav/welcome-tour
|
Thanks for the review @azasypkin 👍 I addressed all remaining IMPORTANT notes. Will merged once CI turns green. |
|
Starting backport for target branches: 8.x https://github.com/elastic/kibana/actions/runs/11345871196 |
💔 All backports failed
Manual backportTo create the backport manually run: Questions ?Please refer to the Backport tool documentation |
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
(cherry picked from commit 8cceaee) # Conflicts: # x-pack/plugins/spaces/common/constants.ts
# Backport This will backport the following commits from `main` to `8.x`: - [[Stateful sidenav] Welcome tour (#194926)](#194926) <!--- Backport version: 8.9.8 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Sébastien Loix","email":"sebastien.loix@elastic.co"},"sourceCommit":{"committedDate":"2024-10-15T12:18:30Z","message":"[Stateful sidenav] Welcome tour (#194926)","sha":"8cceaee0f42c6c0e7ee064ef98a0e652fd77e286","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Feature:Security/Spaces","release_note:skip","v9.0.0","Team:SharedUX","backport:prev-minor"],"number":194926,"url":"https://github.com/elastic/kibana/pull/194926","mergeCommit":{"message":"[Stateful sidenav] Welcome tour (#194926)","sha":"8cceaee0f42c6c0e7ee064ef98a0e652fd77e286"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","labelRegex":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/194926","number":194926,"mergeCommit":{"message":"[Stateful sidenav] Welcome tour (#194926)","sha":"8cceaee0f42c6c0e7ee064ef98a0e652fd77e286"}}]}] BACKPORT-->
In this PR I've added the "Solution view welcome tour".
The tour only shows in the following condition
solutionsetOther rules in place:
Fixes https://github.com/elastic/kibana-team/issues/1142
Screenshots
How to test
kibana.dev.yml/app/management/kibana/spaces/edit/default