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

[Cases] Add Lens markdown plugin #96703

Merged

Conversation

patrykkopycinski
Copy link
Contributor

@patrykkopycinski patrykkopycinski commented Apr 9, 2021

Summary

image (5)
image (6)
image (7)

Untitled_.Apr.9.2021.7_17.PM.mp4

Design links

Whimsical wireframe
Figma

TODO in the follow up PR:

  • Support Lens in Add case page
  • Extract common logic to the package
  • Support Lens in Timeline notes?
  • Allow to Open in Lens just for the preview (disable Save button)

@patrykkopycinski patrykkopycinski added release_note:enhancement v8.0.0 v7.13.0 auto-backport Deprecated - use backport:version if exact versions are needed Feature:Cases-RAC-UI labels Apr 9, 2021
@patrykkopycinski patrykkopycinski self-assigned this Apr 9, 2021
@patrykkopycinski patrykkopycinski requested a review from XavierM April 9, 2021 17:23
@stephmilovic
Copy link
Contributor

This looks awesome Patryk! We are going to be merging the cases_rac_ui branch in the coming days. We had to rework how the markdown plugin takes timeline plugin: #96496

It would be awesome if you could either open this PR against that cases_rac_ui branch or wait until we merge. There will be conflicts for sure. @michaelolo24 or I would be happy to help you through it!

@patrykkopycinski
Copy link
Contributor Author

Thank you for the heads up @stephmilovic :)
This PR is more like PoC to start a discussion with Lens/Explorary view team to discuss possible implementation ways for 7.14, so I'd not worry about having it merged anytime soon :)

@stephmilovic
Copy link
Contributor

well i cant wait to get it merged, it looks really cool!!

@monina-n
Copy link

hi! these are my initial comments based only on the video.

Comments:

  • Visualization lacks a title when added to the case. One idea is to have a title similar to Dashboard styling

Screen Shot 2021-05-28 at 11 04 07 AM

  • Similar comment about the time range. There are timestamps on the X-axis, but it could be helpful to have some type of subtitle so users know at a glance what time range the graph encompasses
  • Beneficial to discuss flyout vs. modal to make sure we're making the right decision

Clarification:

  • Is it only one Lens visualization allowed per comment?
  • Are you able to add additional text in the same comment as the visualization?

patrykkopycinski and others added 4 commits May 29, 2021 20:38
…ns-markdown-plugin

# Conflicts:
#	x-pack/plugins/security_solution/public/common/components/markdown_editor/plugins/index.ts
@stephmilovic
Copy link
Contributor

@elasticmachine merge upstream


const savedObjectMetaData = useMemo(
() => [
{
Copy link
Contributor

Choose a reason for hiding this comment

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

this all looks static, no props? can move outside of component to a plain const

</EuiText>
</EuiFlexItem>
<EuiFlexItem grow={false}>
{viewMode && canUseEditor() ? (
Copy link
Contributor

Choose a reason for hiding this comment

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

i think we can put canUseEditor in a useMemo?

isDisabled={!canUseEditor()}
onClick={handleClick}
>
{`Open visualization`}
Copy link
Contributor

Choose a reason for hiding this comment

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

needs i18n

<EuiSpacer size="xs" />

<EmbeddableComponent
id=""
Copy link
Contributor

Choose a reason for hiding this comment

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

is this correct?

Copy link
Contributor

@stephmilovic stephmilovic left a comment

Choose a reason for hiding this comment

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

Just a few nits and the bugs we discussed over zoom. Remember to add validation to the Visualization Title! LGTM

…ns-markdown-plugin

# Conflicts:
#	x-pack/plugins/security_solution/public/common/components/markdown_editor/editor.tsx
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
cases 263 272 +9
lens 805 806 +1
total +10

Async chunks

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

id before after diff
cases 580.8KB 582.8KB +2.0KB
lens 1.7MB 1.7MB +190.0B
observability 489.2KB 489.3KB +15.0B
osquery 2.3MB 2.3MB +9.0B
securitySolution 6.5MB 6.5MB +951.0B
total +3.1KB

Page load bundle

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

id before after diff
cases 132.1KB 133.0KB +882.0B
lens 29.6KB 30.0KB +433.0B
savedObjects 52.1KB 52.2KB +85.0B
total +1.4KB
Unknown metric groups

API count

id before after diff
cases 452 454 +2
embeddable 467 468 +1
lens 220 246 +26
observability 226 227 +1
savedObjects 220 221 +1
total +31

API count missing comments

id before after diff
cases 410 412 +2
embeddable 391 392 +1
lens 202 228 +26
observability 226 227 +1
savedObjects 206 207 +1
total +31

async chunk count

id before after diff
cases 14 15 +1

Non-exported public API item count

id before after diff
savedObjects 5 4 -1

History

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

cc @patrykkopycinski

@patrykkopycinski patrykkopycinski enabled auto-merge (squash) August 17, 2021 21:17
Copy link

@adamwdraper adamwdraper left a comment

Choose a reason for hiding this comment

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

Summary of sync meeting today between @patrykkopycinski @paulewing @yiyangliu9286 @adamwdraper

Current design as implemented in this pr is not ready to be released as is. It requires iteration of ux to fix UI bugs and align with EUI guidelines. This pr will be merged, but will be behind a feature flag. This feature flag must default to off, unless the new ux is finalized and delivered, and @yiyangliu9286 or @adamwdraper approve the ux matches designs.

Next Steps:

@adamwdraper adamwdraper self-requested a review August 17, 2021 22:48
@patrykkopycinski patrykkopycinski merged commit 754b79f into elastic:master Aug 17, 2021
@kibanamachine
Copy link
Contributor

💔 Backport failed

Status Branch Result
7.x Commit could not be cherrypicked due to conflicts

To backport manually run:
node scripts/backport --pr 96703

@yiyangliu9286
Copy link

@patrykkopycinski Please see design followup issue: #109121 (cc: @adamwdraper @paulewing)

patrykkopycinski added a commit that referenced this pull request Aug 18, 2021
# Conflicts:
#	x-pack/plugins/observability/public/components/shared/exploratory_view/header/header.test.tsx
#	x-pack/plugins/observability/public/components/shared/exploratory_view/header/header.tsx
#	x-pack/plugins/observability/public/plugin.ts
patrykkopycinski added a commit to patrykkopycinski/kibana that referenced this pull request Aug 18, 2021
@patrykkopycinski patrykkopycinski deleted the feat/cases-lens-markdown-plugin branch August 24, 2021 00:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Embedding Embedding content via iFrame release_note:enhancement v7.15.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.