-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
[Guided onboarding] Update header button logic #144634
[Guided onboarding] Update header button logic #144634
Conversation
59ae0d3
to
f6be0a0
Compare
f6be0a0
to
5165618
Compare
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.
Nice work @yuliacech! Just took an initial glance through, as I know you are still working on a few things. This approach makes sense to me overall. Did not test locally yet. I will take another pass tomorrow.
// https://github.com/elastic/kibana/issues/139799, https://github.com/elastic/kibana/issues/139798 | ||
|
||
// if there is no active guide | ||
if (!pluginState || !pluginState.activeGuide || !pluginState.activeGuide.isActive) { |
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.
Do we need the isActive
flag anymore since the plugin state provides the activeGuide
? In other words, would the activeGuide
ever not be active? 🤔
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.
I thought about that too, but I think we still need the flag isActive
because guides are a separate saved object type and we keep all guides with their states in there. The only way to know which is the active guide is by querying the flag. Alternatively, we could keep an ID of the active guide in the plugin state SO, but I think the query works pretty well and is an already "tested" approach.
|
||
await updateComponentWithState(component, mockInProgressTestGuideState, false); | ||
describe('when not in active period', () => { |
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.
Does it make sense to add a test that the button is still visible if a guide is in progress and not in the active period?
const { id, status } = step; | ||
|
||
if (status === 'ready_to_complete') { | ||
return await api.completeGuideStep(guideState?.guideId, id); | ||
return await api.completeGuideStep(pluginState!.activeGuide!.guideId!, id); |
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.
[nit] Maybe extract this on L91 since it's used in a few places:
const guideId = pluginState!.activeGuide!.guideId!
@@ -0,0 +1,35 @@ | |||
/* |
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.
Should this file be renamed plugin_state_utils.test.ts
?
} from '../saved_objects'; | ||
|
||
// hard code the duration to 30 days for now | ||
const activePeriodDurationInMilliseconds = 30 * 24 * 60 * 60 * 1000; |
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.
Should we open up a separate issue to make this configurable?
* Side Public License, v 1. | ||
*/ | ||
|
||
import { GuideState } from '@kbn/guided-onboarding'; |
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.
Can you remind me again why GuideState
lives in a package and not in the plugin?
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.
We needed GuideState
in the Home plugin and then moved some of the code from the Home plugin into a package, so the types have to be in the package too.
Pinging @elastic/platform-onboarding (Team:Journey/Onboarding) |
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.
limits.yml
net diff is 1kb, update_limits is padding 15kb.
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.
Thanks for addressing my feedback! Verified changes locally and everything worked as expected.
I have one question about the "quit" behavior - my memory is a little fuzzy 😅, but I thought the button would be hidden in the scenario where the user quit a guide. Can you confirm with design? I'm OK to merge as-is and follow up later if necessary.
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 in CI green
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
ESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
* main: (65 commits) Migrate server-side `Root` and `Server` to packages (elastic#144990) [Discover] Handle no data views state for `esQuery` alert (elastic#145052) [ML] Allow updates for number of allocations and priority for trained model deployments (elastic#144704) [api-docs] 2022-11-15 Daily api_docs build (elastic#145203) [Security solution] remove guided onboarding feature flag (elastic#144247) [DOCS] Automate final case APIs (elastic#145007) [Enterprise Search] Name and description flyout for connectors (elastic#143827) [Guided onboarding] Update header button logic (elastic#144634) [Lens] Multi metric partition charts (elastic#143966) [Dashboard] [Controls] Add unmapped runtime field support to options list (elastic#144947) [Security Solution] Add Task Metric Collection to New Tasks (elastic#145181) [TriggersActionsUi] disable jest config in CI (elastic#145186) [TableListView] Enhance tag filtering (elastic#142108) [Cloud Posture] Compliance by CIS section table (elastic#145114) [8.6][Session View] Fix hidden alert flyout in session view (elastic#145141) [customIntegrations] async load all components (elastic#145166) Fix time for logs smoke tests in integration test (elastic#145130) [RAM] Update rule status (elastic#140882) Update babel (main) (elastic#145060) [Actionable Observability] Add context.alertDetailsUrl variable to action connector template for APM rule types (elastic#144791) ...
Summary
Fixes #141129
Fixes #144515
This PR introduces a new state to the guided onboarding plugin. The state keeps track of the
creationDate
and of the overallstatus
of the plugin. The creation date allows us to detect an "active" period during which the header button will be displayed more prominently in the header. Currently, the active period is set to 30 days. During this time, if the user has not started any guide or skipped the guide on the landing page, the header button will be displayed and when clicked, redirect the user to the landing page to start/continue a guide. The button is hidden when the user completed a guide or they quit before completion.Also this PR adds a check for Cloud deployments and prevents the code from sending any API requests when not on Cloud, because guided onboarding is disabled on prem.
Screenshot
Checklist