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

[Guided onboarding] Dynamic URLs for steps #154572

Merged
merged 9 commits into from
Apr 21, 2023

Conversation

yuliacech
Copy link
Contributor

@yuliacech yuliacech commented Apr 6, 2023

Summary

Fixes #143322

This PR adds an option to store dynamic parameters when a step is completed to be used later for dynamically built URLs.

How to test

  1. Add xpack.cloud.id: 'testID' to your /config/kibana.dev.yml file
  2. Start ES with yarn es snapshot
  3. Start Kibana with yarn start --run-examples
  4. Navigate to the guided onboarding example plugin http://localhost:5601/app/guidedOnboardingExample
  5. Start the test guide and when completing step 1 provide any value for the paramter indexName
  6. Continue the guide until step 4 and check that the correct value is being used for the url of this step.

Checklist

@yuliacech yuliacech added release_note:skip Skip the PR/issue when compiling release notes Team:Journey/Onboarding Platform Journey Onboarding team labels Apr 6, 2023
@yuliacech
Copy link
Contributor Author

@elasticmachine merge upstream

@yuliacech yuliacech marked this pull request as ready for review April 18, 2023 15:37
@yuliacech yuliacech requested a review from a team as a code owner April 18, 2023 15:37
@elasticmachine
Copy link
Contributor

Pinging @elastic/platform-onboarding (Team:Journey/Onboarding)

@yuliacech yuliacech changed the title [WIP][Guided onboarding] Dynamic URLs for steps [Guided onboarding] Dynamic URLs for steps Apr 18, 2023
@alisonelizabeth alisonelizabeth self-requested a review April 19, 2023 15:58
Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @yuliacech! Changes LGTM. Verified locally. Left a couple minor comments in the code. Let me know if you end up making changes and want me to review again.

Also, noticed a small UI bug while testing (unrelated to your PR). Opened #155421 to follow up.

export interface GuideState {
guideId: GuideId;
status: GuideStatus;
isActive?: boolean; // Drives the current guide shown in the dropdown panel
steps: GuideStep[];
params?: GuideParams;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we rename this to something more specific? Or do we expect the potential to use this in other ways than just the URL in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I thought that we might have other dynamic parameters like integrations or something in the copy. But I can also rename it to something more specific, for example urlParams or even just guideConfigParams or guideParams, wdyt? (naming is hard 😅 )

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm OK to leave as-is for now. We can always revisit.

@@ -360,7 +361,8 @@ export class ApiService implements GuidedOnboardingApi {
*/
public async completeGuideStep(
guideId: GuideId,
stepId: GuideStepIds
stepId: GuideStepIds,
params?: GuideParams
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] Probably outside the scope of this PR, but this might be easier to reason with if we passed in an single object with the arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reasoning here was to not introduce any breaking changes for the existing guides that are not going to use dynamic params.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 makes sense

@yuliacech
Copy link
Contributor Author

Thanks a lot for your review, @alisonelizabeth!
I did end up doing a small change: removing the new params property from the location object, because we should just be able to match any dynamic params inside curly braces in the path string itself. Otherwise, consumers could forget to update the property params when they add a dynamic param to the path. The commit is here, could you please have another look?

@alisonelizabeth
Copy link
Contributor

I did end up doing a small change: removing the new params property from the location object, because we should just be able to match any dynamic params inside curly braces in the path string itself. Otherwise, consumers could forget to update the property params when they add a dynamic param to the path. The commit is here, could you please have another look?

Thanks @yuliacech! That makes sense to me. I reviewed the commit and changes LGTM. I did not retest.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
guidedOnboarding 28 29 +1

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/guided-onboarding 50 52 +2
guidedOnboarding 55 56 +1
total +3

Page load bundle

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

id before after diff
guidedOnboarding 26.9KB 27.2KB +267.0B
Unknown metric groups

API count

id before after diff
@kbn/guided-onboarding 52 54 +2
guidedOnboarding 56 57 +1
total +3

ESLint disabled line counts

id before after diff
enterpriseSearch 16 18 +2
securitySolution 394 397 +3
total +5

Total ESLint disabled count

id before after diff
enterpriseSearch 17 19 +2
securitySolution 474 477 +3
total +5

History

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

@yuliacech yuliacech merged commit bc3471c into elastic:main Apr 21, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Apr 21, 2023
@yuliacech yuliacech deleted the guided_onboarding/8.8/dynamic_steps branch February 15, 2024 12:04
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 release_note:skip Skip the PR/issue when compiling release notes Team:Journey/Onboarding Platform Journey Onboarding team v8.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Guided onboarding] Support dynamic step URLs
5 participants