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

Nav unified show timeline #131811

Merged
merged 6 commits into from
May 18, 2022
Merged

Conversation

machadoum
Copy link
Member

@machadoum machadoum commented May 9, 2022

Summary

Update useShowTimeline to read timeline status from app links config when GroupedNavigation is enabled.

  • It adds SecurityPageName.rulesCreateto deep links.

Checklist

Delete any items that are not applicable to this PR.

@machadoum machadoum force-pushed the nav_unified_show_timeline branch from 48076f6 to 0eed070 Compare May 11, 2022 13:43
@machadoum machadoum marked this pull request as ready for review May 11, 2022 13:51
@machadoum machadoum requested review from a team as code owners May 11, 2022 13:51
@machadoum machadoum self-assigned this May 11, 2022
@machadoum machadoum added release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting Security Solution Threat Hunting Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Threat Hunting:Explore labels May 11, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-threat-hunting (Team:Threat Hunting)

@machadoum machadoum requested a review from semd May 11, 2022 13:51
@machadoum machadoum force-pushed the nav_unified_show_timeline branch 5 times, most recently from c35ca17 to 2ccf8c4 Compare May 16, 2022 09:59
@machadoum machadoum requested a review from semd May 16, 2022 11:33
@machadoum machadoum force-pushed the nav_unified_show_timeline branch from 2dfcde3 to a4dac0e Compare May 16, 2022 11:48
Copy link
Contributor

@semd semd left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@machadoum machadoum force-pushed the nav_unified_show_timeline branch from ed17d15 to 679ac04 Compare May 17, 2022 11:07
@@ -439,7 +439,7 @@ const CreateRulePageComponent: React.FC = () => {
</EuiFlexGroup>
</SecuritySolutionPageWrapper>

<SpyRoute pageName={SecurityPageName.rules} />
<SpyRoute pageName={SecurityPageName.rulesCreate} />
Copy link
Member Author

Choose a reason for hiding this comment

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

Add rulesCreate to SpyRoute so we can disable the timeline.

Copy link
Contributor

Choose a reason for hiding this comment

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

Tested this locally -- LGTM 👍
And thank you for these comments, they are really helpful for code review.

@@ -80,14 +67,13 @@ const PrePackagedRulesPromptComponent: React.FC<PrePackagedRulesPromptProps> = (
<EuiFlexGroup justifyContent="center">
<EuiFlexItem grow={false}>{loadPrebuiltRulesAndTemplatesButton}</EuiFlexItem>
<EuiFlexItem grow={false}>
<LinkButton
<SecuritySolutionLinkButton
Copy link
Member Author

Choose a reason for hiding this comment

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

A small refactor.

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks so much cleaner! 🙌 Thank you!

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
securitySolution 5.0MB 5.0MB +574.0B

Page load bundle

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

id before after diff
securitySolution 246.7KB 247.0KB +328.0B

History

  • 💚 Build #44946 succeeded ed17d1574876d501ff6154cb2d88d699ca26524f
  • 💚 Build #44864 succeeded a4dac0e09db3044527df93cf654d9720969bad46
  • 💚 Build #44823 succeeded 2ccf8c40bf78e12ba9da624b99a07a8425bf0bfb
  • 💔 Build #44517 failed c35ca1716db1eddcd2a0460bb6504ddc1f31d19f
  • 💔 Build #44449 failed bc2eae55b1cc63bffa7c9d83658fa160799971a1
  • 💔 Build #44422 failed 1a33efdd4427620fd4a51bb993916781f3f2f150

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

cc @machadoum

Copy link
Contributor

@paul-tavares paul-tavares left a comment

Choose a reason for hiding this comment

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

file x-pack/plugins/security_solution/public/management/links.ts changes LGTM 👍

Copy link
Member

@joeypoon joeypoon left a comment

Choose a reason for hiding this comment

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

👌

@machadoum machadoum enabled auto-merge (squash) May 18, 2022 07:34
Copy link
Contributor

@banderror banderror left a comment

Choose a reason for hiding this comment

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

Rules-related changes LGTM 👍 Tested a bit locally. Thank you @machadoum!

expect(
wrapper.find('[data-test-subj="load-prebuilt-rules"] button').props().disabled
).toEqual(true);
expect(wrapper.find('button[data-test-subj="load-prebuilt-rules"]').props().disabled).toEqual(
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: could we just use [data-test-subj="load-prebuilt-rules"] w/o button as a selector here?

@@ -80,14 +67,13 @@ const PrePackagedRulesPromptComponent: React.FC<PrePackagedRulesPromptProps> = (
<EuiFlexGroup justifyContent="center">
<EuiFlexItem grow={false}>{loadPrebuiltRulesAndTemplatesButton}</EuiFlexItem>
<EuiFlexItem grow={false}>
<LinkButton
<SecuritySolutionLinkButton
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks so much cleaner! 🙌 Thank you!

@@ -439,7 +439,7 @@ const CreateRulePageComponent: React.FC = () => {
</EuiFlexGroup>
</SecuritySolutionPageWrapper>

<SpyRoute pageName={SecurityPageName.rules} />
<SpyRoute pageName={SecurityPageName.rulesCreate} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Tested this locally -- LGTM 👍
And thank you for these comments, they are really helpful for code review.

@machadoum machadoum merged commit fa7df79 into elastic:main May 18, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label May 18, 2022
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: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Threat Hunting:Explore Team:Threat Hunting Security Solution Threat Hunting Team v8.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants