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] Search guide copy #144234

Merged
merged 15 commits into from
Nov 2, 2022

Conversation

yuliacech
Copy link
Contributor

@yuliacech yuliacech commented Oct 31, 2022

Summary

Partially addresses #139741

This PR updates the config for the Search guide:

  • The copy is reviewed by @kellyemurphy
  • Removed the docs link for 8.6 release
  • Updated the guide to only 2 steps

Screenshot

Screenshot 2022-11-02 at 14 28 13

@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 31, 2022
@@ -216,7 +216,8 @@ export const Main = (props: MainProps) => {
)}
{(guideState?.isActive === true ||
guideState?.status === 'in_progress' ||
guideState?.status === 'ready_to_complete') && (
guideState?.status === 'ready_to_complete' ||
guideState?.status === 'not_started') && (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not_started status was missing from the examples page

@@ -21,50 +21,50 @@ import { registerTestBed, TestBed } from '@kbn/test-jest-helpers';

const applicationMock = applicationServiceMock.createStartContract();

const mockActiveSearchGuideState: GuideState = {
guideId: 'search',
const mockActiveTestGuideState: GuideState = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated the jest test to use the test guide instead of search

})}
</ul>
)}
{stepConfig.description && <p>{stepConfig.description}</p>}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a space between the paragraph and the list so I might need to wait until Cindy's PR is in and use the css to remove the margin.

@yuliacech yuliacech marked this pull request as ready for review October 31, 2022 16:10
@yuliacech yuliacech requested a review from a team as a code owner October 31, 2022 16:10
@elasticmachine
Copy link
Contributor

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

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! Did not test locally. Code looks good overall. I did leave a comment around naming, and also just wanted to confirm the behavior when description is provided as well as descriptionList with only 1 item. If you could take a look before merging that would be great.

@@ -54,7 +54,8 @@ export interface GuidedOnboardingApi {
export interface StepConfig {
id: GuideStepIds;
title: string;
descriptionList: string[];
description?: string;
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 add some code comments here to explain the difference between description and descriptionList? I could see how it could be confusing when to use which, especially since descriptionList renders as a paragraph if only one item is provided (thinking the future, when more teams are registering guides 😄).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some comments to the types and updated the step to always render descriptionList as ul. With introduction of description it doesn't make much sense to account for the list with only 1 item, I think.

})}
</ul>
)}
{stepConfig.description && <p>{stepConfig.description}</p>}
Copy link
Contributor

Choose a reason for hiding this comment

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

If a guide config contains the description field and descriptionList with only 1 item in the array, does it still render as expected (i.e., two

tags)?

Copy link
Contributor

@kellyemurphy kellyemurphy left a comment

Choose a reason for hiding this comment

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

I do want Casey's ok on the changes to those final bullet points, but overall just some small tweaks and missing punctuation.

defaultMessage: 'Build a search experience',
}),
description: i18n.translate('guidedOnboarding.searchGuide.searchExperienceStep.description', {
defaultMessage: 'Take a tour of Elastic’s relevance refinement tools, including:',
Copy link
Contributor

Choose a reason for hiding this comment

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

@zumwalt can you please review my suggestions here? I just realized we're mentioning relevance on every line.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch! I suggested the correct copy for this step.

@yuliacech
Copy link
Contributor Author

Thank you for the review, @alisonelizabeth! Good catch with description and descriptionList, addressed your comments in the latest commit.

@yuliacech
Copy link
Contributor Author

Thanks a lot for you review and suggestions, @kellyemurphy! I'll commit your text changes and update the screenshots in the PR description.

Copy link
Contributor

@zumwalt zumwalt left a comment

Choose a reason for hiding this comment

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

@yuliacech I suggested changes to the "Build a Search Experience" step

defaultMessage: 'Build a search experience',
}),
description: i18n.translate('guidedOnboarding.searchGuide.searchExperienceStep.description', {
defaultMessage: 'Take a tour of Elastic’s relevance refinement tools, including:',
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove this in favor of a simple description list

@yuliacech yuliacech enabled auto-merge (squash) November 2, 2022 13:29
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Page load bundle

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

id before after diff
guidedOnboarding 23.8KB 23.9KB +61.0B
Unknown metric groups

ESLint disabled in files

id before after diff
osquery 1 2 +1

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
fleet 57 63 +6
osquery 103 108 +5
securitySolution 439 445 +6
total +19

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
fleet 65 71 +6
osquery 104 110 +6
securitySolution 516 522 +6
total +20

History

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

@yuliacech yuliacech merged commit 92db69c into elastic:main Nov 2, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Nov 2, 2022
@yuliacech yuliacech deleted the guided_onboarding/8.6_search_copy branch November 22, 2022 09:32
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.

7 participants