-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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] Setup guide panel component #140836
[Guided onboarding] Setup guide panel component #140836
Conversation
a9a5dcf
to
85ee1ab
Compare
@@ -26,7 +26,7 @@ import { | |||
} from '@elastic/eui'; | |||
import { | |||
GuidedOnboardingPluginStart, | |||
GuidedOnboardingState, | |||
SetupGuideState, |
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 renamed this interface and other areas in the code to "setupGuide..." to better align with the name of the actual component. However, if you feel this causes more confusion than not (i.e., the plugin is still called guided_onboarding
), I can revert this change.
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.
Hi @alisonelizabeth, thanks a lot for cleaning up the code! Changes LGTM.
About the naming of the variables and components: I think the word "setup" is a bit confusing because for me it has something to do with the plugin setup. So for the component I'm thinking if "checklist" might be better? And for the state, for now "GuidedOnboardingState" seems clearer to me. Maybe though we could use "guided setup" in both cases instead?
Thanks @yuliacech for the review! I ended up leaving I also added some very lightweight component tests. I expect to build upon these as we continue working on this project. |
Pinging @elastic/platform-onboarding (Team:Journey/Onboarding) |
@elasticmachine merge upstream |
@alisonelizabeth: I checked this locally and overall it looks good to me 🙂, there are two things I wanted to note (please ignore if these are to be added later):
|
Thanks for the feedback @cindychangy!
Thanks for pointing this out! I will address this in a follow-up PR. I've opened #141223 to track misc design feedback.
👍 This should be addressed as part of #140981. Let me know if I missed anything though in the issue description. |
…ng/panel_component
💚 Build Succeeded
Metrics [docs]Module Count
Page load bundle
History
To update your PR or re-run it, just comment with: |
This PR polishes the setup guide button and dropdown panel to align with latest design.
Addresses #138557, #139751
How to test
yarn start --run-examples
/app/guidedOnboardingExample
What's missing
This PR is the first step to polish the setup guide component. There is additional work that will be addressed in follow-up PRs:
Screenshots