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] Landing page updates #143194

Merged
merged 43 commits into from
Oct 20, 2022

Conversation

yuliacech
Copy link
Contributor

@yuliacech yuliacech commented Oct 12, 2022

Summary

Fixes #138548

This PR updates the Guided onboarding Landing page that can be found at /app/home#/getting_started. The Landing page has now 4 cards: 3 solution guides and a link to Infrastructure category. The guide card displays the state of each guide with the progress bar and a button to "start/continue" the guide.

Note: The "view integrations" button links to /app/integrations/browse/infrastructure but the category doesn't have any integrations added to it yet. As soon as there are any integrations in that category, the link should work as expected.

Screenshots

No guides started yet

The header button is disabled, guide cards have "view guide" buttons.

Screenshot 2022-10-19 at 18 46 32

"View guide" button is clicked

The dropdown panel shows the selected guide and the user can start the first step. The header button is enabled.

Screenshot 2022-10-19 at 18 46 56

A guide is in progress

The guide card displays the progress and the "continue" button.

Screenshot 2022-10-19 at 18 47 27

A guide has been completed

The guide card displays the completed progress bar and the "view guide" button.

Screenshot 2022-10-19 at 18 48 27

How to test

  1. Enable the guided onboarding plugin by adding guidedOnboarding.ui: true to the file /config/kibana.dev.yml.
  2. Start ES and Kibana with examples: yarn es snapshot and yarn start --run-examples.
  3. Navigate to /app/home#/getting_started and check the landing page when no guides have been started yet.
  4. Click the card for the search guide and check that the dropdown panel is being displayed and the first step can be started.
  5. Start the first step and complete it on the page /app/guidedOnboardingExample/stepOne.
  6. Navigate back to the landing page and check that the search card now displays the progress.
  7. Complete the search guide and check that the landing page displays the search guide as completed.

Checklist

@yuliacech yuliacech marked this pull request as ready for review October 12, 2022 17:07
@yuliacech yuliacech requested a review from a team as a code owner October 12, 2022 17:07
@yuliacech yuliacech requested a review from a team October 12, 2022 17:07
@yuliacech yuliacech added release_note:skip Skip the PR/issue when compiling release notes Team:Journey/Onboarding Platform Journey Onboarding team v8.6.0 labels Oct 12, 2022
@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@cindychangy cindychangy left a comment

Choose a reason for hiding this comment

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

Thanks @yuliacech, this looks good! I left a couple of small style tweaks. They can move to the larger design polish task as well if preferred.

@@ -86,15 +99,18 @@ export const GettingStarted = () => {
</EuiText>
<EuiSpacer size="s" />
<EuiSpacer size="xxl" />
<EuiFlexGrid columns={3} gutterSize="xl">
<EuiFlexGrid columns={4} gutterSize="l">
Copy link
Contributor

Choose a reason for hiding this comment

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

I was unable to insert directly on line 97 but can we slightly modify the line to read: <EuiText size="m" textAlign="center">

Copy link
Contributor

@majagrubic majagrubic left a comment

Choose a reason for hiding this comment

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

Thanx for extracting this into a package. I left some comments around the code and the home plugin specifcially.

)}
/>
<EuiSpacer size="l" />
<div className="eui-textCenter">
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this for? We shouldn't be using eui class names directly. Looks like you need a flexGroup?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I followed your suggestion to not use a flexGroup when there is only 1 component. Please let me know if there is a better way to center the button in the footer.

Copy link
Contributor

@majagrubic majagrubic Oct 19, 2022

Choose a reason for hiding this comment

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

Sorry, maybe my comment wasn't clear initially. We shouldn't be using eui- classes directly in our React components, as they are internal to EUI and could possibly change. What I meant was - you can use EuiFlexGroup to achieve centering of the button (it's fine even for one component). Another way is to set the div width to 50% and margin to auto:
https://jsfiddle.net/79kjtwqo/1/

Copy link
Contributor

@alisonelizabeth alisonelizabeth Oct 19, 2022

Choose a reason for hiding this comment

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

I don't think eui-textCenter is internal to EUI, or at least is is documented as an available utility class here - https://elastic.github.io/eui/#/theming/typography/utilities. I would argue it is OK to use here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with Alison here, if the EUI class should not be used in Kibana, there should be an eslint rule for that.

Copy link
Contributor

@clintandrewhall clintandrewhall Oct 20, 2022

Choose a reason for hiding this comment

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

@majagrubic was absolutely correct here.

@alisonelizabeth it's not that it's internal to EUI, it's that they're likely not going to exist for much longer. Even on the documentation link you reference, it states:

For most of your usages we recommend using EuiText or EuiTitle components directly instead of these theme tokens. Or head to the text utilities page for helper classes and functions.

EUI is moving to Emotion:

EUI is transitioning to a CSS-in-JS solution for theming and requires consuming/customizing global variables in both the Sass and CSS-in-JS methods. To track this effort, subscribe to the Meta ticket.

Using these classes directly should be for edge cases only. The right solution for this would have been to use EuiText, rather than a div with a global EUI classname.

@yuliacech The reason there's no lint rule is that it would literally blow up the linter, due to the number of exceptions we've made in the past, (or older cases before Emotion and theming).

I would recommend a follow-up PR to switch to EuiText:

https://elastic.github.io/eui/#/display/text#alignment

There are two ways to align text. Either individually by applying EuiTextAlign on individual text objects, or by passing the textAlign prop directly on EuiText for a blanket approach across the entirety of your text.

Copy link
Contributor

Choose a reason for hiding this comment

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

@alisonelizabeth it's not that it's internal to EUI, it's that they're likely not going to exist for much longer.

👍 thanks for clarifying

The right solution for this would have been to use EuiText, rather than a div with a global EUI classname.

This is for centering a button though, right @yuliacech? I don't think EuiText is appropriate in that case. Perhaps @majagrubic earlier suggestion is the best approach:

What I meant was - you can use EuiFlexGroup to achieve centering of the button (it's fine even for one component). Another way is to set the div width to 50% and margin to auto:
https://jsfiddle.net/79kjtwqo/1/

I've opened #143794 to follow up on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks a lot for the context, @clintandrewhall! I just wish this were communicated more broadly because with the number of Kibana contributors we won't be able to address the issue via individual code reviews. An eslint rule might be a grind at the beginning to disable all lines with classes, but at least it would prevent new ones from being added. Otherwise a documentation update might be an easier alternative. From the text, it's not clear that the classes will be deleted soon and currently should not be used in production, maybe a deprecation could be added there?

fill
onClick={() => activateGuide(useCase, guideState)}
>
{i18n.translate(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be a constant as well?

value={numberCompleteSteps}
max={numberSteps}
size="s"
label={i18n.translate(
Copy link
Contributor

Choose a reason for hiding this comment

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

Constant

)}
/>
<EuiSpacer size="l" />
<div className="eui-textCenter">
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question as above

import { GuideId, GuideState } from '../../types';
import { UseCase } from './use_case_card';

export const GuideCardFooter = ({
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like there are not tests for this component.

setGuidesState(allGuides.state);
}
}, [guidedOnboardingService]);
useEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: please add space between useEffect and previous line.

const isDarkTheme = uiSettings.get<boolean>('theme:darkMode');
const activateGuide = async (useCase: UseCase, guideState?: GuideState) => {
await guidedOnboardingService?.activateGuide(useCase as GuideId, guideState);
// TODO error handling
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's do it now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't have the correct error handling in the API yet, so this will be addressed in a separate issue

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you link the issue in the TODO then?

</EuiFlexItem>
<EuiFlexItem>
<UseCaseCard useCase="security" />
<GuideCard
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we render these cards in a loop? It looks like (almost) all are the same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to display a guide card for search, a guide card for observability, a link card for observability and a guide card for security, so I don't think a loop would work with this logic.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would something like this not work?

{ ['search', 'observability', 'observabilityLink', 'security'].map((useCase) => {
      if (useCase === 'observabilityLink') { 
         return (<EuiFlexItem>
                <ObservabilityLinkCard
                  ....
                />
            </EuiFlexItem>) ;
      } 
      return (<EuiFlexItem>
         <GuideCard useCase={useCase} .../>
       </EuiFlexItem>);
    };
}

@yuliacech
Copy link
Contributor Author

Hi @majagrubic, thanks a lot for your review! I addressed your feedback in 0d46ae3, could you please have another look?

@kellyemurphy
Copy link
Contributor

kellyemurphy commented Oct 19, 2022

Looking back at the design files, there are more differences in the text here.

(Plus "Search my Data" should be "Search my data", which you have correct in your PR)

Screen Shot 2022-10-19 at 11 21 43 AM

yuliacech and others added 2 commits October 19, 2022 18:31
…ervability_link_card.tsx

Co-authored-by: Kelly Murphy <kelly.murphy@elastic.co>
@yuliacech
Copy link
Contributor Author

Thanks a lot, @kellyemurphy! I used the figma file you linked and updated the texts, the screenshots in the PR description should be the latest state as well.

@alisonelizabeth
Copy link
Contributor

@elasticmachine merge upstream

@yuliacech yuliacech enabled auto-merge (squash) October 20, 2022 08:09
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
home 175 180 +5

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 - 17 +17
guidedOnboarding 19 36 +17
total +34

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
home 147.8KB 151.0KB +3.2KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
@kbn/guided-onboarding - 2 +2
guidedOnboarding 3 1 -2
total -0

Page load bundle

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

id before after diff
home 10.8KB 10.8KB +65.0B
Unknown metric groups

API count

id before after diff
@kbn/guided-onboarding - 17 +17
guidedOnboarding 19 36 +17
total +34

History

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

Copy link
Contributor

@majagrubic majagrubic 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 addressing the comments. Home plugin changes look good.

@yuliacech yuliacech merged commit 9c0dd18 into elastic:main Oct 20, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Oct 20, 2022
Copy link
Contributor

@clintandrewhall clintandrewhall left a comment

Choose a reason for hiding this comment

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

A few other comments while looking over this PR after the fact. YMMV. Feel free to reach out if Global Experience can help in any way!

// Used for FS tracking
data-test-subj={`onboarding--guideCard--view--${useCase}`}
fill
onClick={() => activateGuide(useCase, guideState)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Incidentally, this is also a problem: you're creating a new function on every render cycle.

// Used for FS tracking
data-test-subj={`onboarding--guideCard--continue--${useCase}`}
fill
onClick={() => activateGuide(useCase, guideState)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same.

activateGuide: (useCase: UseCase, guideState?: GuideState) => void;
}
export const GuideCardFooter = ({ guides, useCase, activateGuide }: GuideCardFooterProps) => {
const guideState = guides.find((guide) => guide.guideId === (useCase as GuideId));
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels like a code smell-- casting a UseCase as a GuideId, and that UseCase and GuideId are identical...?

},
};

export type UseCase = 'search' | 'observability' | 'security';
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 curious why this type is necessary, when it matches GuideId. Appears to me these types need coalescing.

e40pud pushed a commit to e40pud/kibana that referenced this pull request Oct 24, 2022
* [Guided onboarding] Updated landing page

* [Guided onboarding] Finished landing page changes

* [Guided onboarding] Fixed card for completed guides

* [Guided onboarding] Fixed types errors

* [Guided onboarding] Fixed i18n issues

* Update src/plugins/home/public/application/components/guided_onboarding/use_case_card.tsx

Co-authored-by: Cindy Chang  <cindyisachang@gmail.com>

* Update src/plugins/home/public/application/components/guided_onboarding/use_case_card.tsx

Co-authored-by: Cindy Chang  <cindyisachang@gmail.com>

* Update src/plugins/home/public/application/components/guided_onboarding/use_case_card.tsx

Co-authored-by: Cindy Chang  <cindyisachang@gmail.com>

* [Guided onboarding] Added CR comments

* [Guided onboarding] Added view guide button to the completed guide

* [Guided onboarding] Fixed the typo in kibana services

* [Guided onboarding] Started moving the components out of home plugin into the guided onboarding package

* [Guided onboarding] Fix the imports in the plugin

* [Guided onboarding] Fix the tests in the new package

* [CI] Auto-commit changed files from 'node scripts/generate codeowners'

* [Guided onboarding] Fix the package file and the yarn.lock file

* [Guided onboarding] Fix the build

* [Guided onboarding] More refactoring

* [Guided onboarding] More refactoring

* [Guided onboarding] More refactoring

* [Guided onboarding] More refactoring of types

* [CI] Auto-commit changed files from 'node scripts/precommit_hook.js --ref HEAD~1..HEAD --fix'

* [Guided onboarding] More refactoring of types

* [Guided onboarding] Fix the types issues

* [Guided onboarding] Update the tests for the api

* [Guided onboarding] Fixed the i18n errors

* [Guided onboarding] Fixed the i18n errors

* [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix'

* [Guided onboarding] Fixed the jest tests

* [Guided onboarding] Home changes

* Update packages/kbn-guided-onboarding/src/components/landing_page/observability_link_card.tsx

Co-authored-by: Kelly Murphy <kelly.murphy@elastic.co>

* [Guided onboarding] Address copy feedback

* [Guided onboarding] Address CR feedback

Co-authored-by: Cindy Chang  <cindyisachang@gmail.com>
Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Kelly Murphy <kelly.murphy@elastic.co>
@yuliacech yuliacech deleted the guided_onboarding/8.6_landing_page branch November 22, 2022 09:31
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.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Guided onboarding] Onboarding landing page improvements
10 participants