Skip to content

[APM] Use Code preview component for displaying stackframes#47974

Closed
dgieselaar wants to merge 4 commits intoelastic:masterfrom
dgieselaar:integrate-codesearch-ui
Closed

[APM] Use Code preview component for displaying stackframes#47974
dgieselaar wants to merge 4 commits intoelastic:masterfrom
dgieselaar:integrate-codesearch-ui

Conversation

@dgieselaar
Copy link
Copy Markdown
Contributor

@dgieselaar dgieselaar commented Oct 11, 2019 •
edited
Loading

Closes #47059.

Blocked by:

  • highlighting. current highlight functionality is intended for search results rather than APM's usage (line highlighting). Code will extend CodeBlock to support this as well.
  • imports should point to code/public, but importing that file currently leads to the Code application taking over APM's routing.

Before:
image

After:
image

@dgieselaar dgieselaar requested a review from a team October 11, 2019 14:53
@dgieselaar dgieselaar force-pushed the integrate-codesearch-ui branch from 50d38b0 to bf6bfba Compare October 11, 2019 14:55
@sorenlouv
Copy link
Copy Markdown
Contributor

sorenlouv commented Oct 11, 2019 •
edited
Loading

Oh man, I just love pull requests like this 😍
image

@sorenlouv
Copy link
Copy Markdown
Contributor

sorenlouv commented Oct 11, 2019 •
edited
Loading

This looks great! Any obstacles to shipping this in 7.5?

nit: I prefer the highlight style from before, that expands to the entirety of the line and is not quite as strong. What does the design team on code think about that? @daveyholler @formgeist

Edit: it also looks like syntax highlighting disappears on highlighted lines.

Btw. Github's highlighting is pretty sleek:
image

@daveyholler
Copy link
Copy Markdown
Contributor

daveyholler commented Oct 11, 2019

@sqren I agree that the bright yellow highlight is a little heavy for line highlighting. It was originally intended to highlight the searched-for term in Code. Our line highlighting in Code is a bit different.
image

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Oct 11, 2019

💔 Build Failed

@sorenlouv
Copy link
Copy Markdown
Contributor

sorenlouv commented Oct 11, 2019

@daveyholler Much better. How do we get that style?

@daveyholler
Copy link
Copy Markdown
Contributor

daveyholler commented Oct 11, 2019

@sqren That's a good question.

This is ultimately using the monaco component which handles the line highlighting with these lines in particular:

123: selectionBackground: `${IS_DARK_THEME ? '#343551' : '#E3E4ED'}`,

···

200: 'editor.selectionBackground': syntaxTheme.selectionBackground,

I'd assume that this isn't using traditional line highlighting, but rather some kind of selection highlighting or the codeSearch__highlight className:

.codeSearch__highlight {
    background-color: #E6C220;
    color: black !important;
    padding: 2px;
    border-radius: 2px;
    font-weight: bold;
    font-style: oblique;
    cursor: pointer;
}

mw-ding
mw-ding reviewed Oct 11, 2019
View reviewed changes
x-pack/legacy/plugins/apm/public/components/shared/Stacktrace/Stackframe.tsx Outdated
Copy link
Copy Markdown
Contributor

@mw-ding mw-ding Oct 11, 2019

Choose a reason for hiding this comment

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

@rylnd can you suggest on how to embed this CodeHeader and Variables into the CodeBlock component?

Copy link
Copy Markdown
Contributor

@rylnd rylnd Oct 11, 2019

Choose a reason for hiding this comment

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

I think for now, simply making CodeHeader and Variables siblings like this means less responsibility for CodeBlock, and an identical amount of work for the consumer. Within Code, we're going to continue to use CodeBlockPanel which accepts a header prop, but even that is of dubious value.

Copy link
Copy Markdown
Contributor

@mw-ding mw-ding Oct 14, 2019

Choose a reason for hiding this comment

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

sg

@dgieselaar dgieselaar force-pushed the integrate-codesearch-ui branch from bf6bfba to e416837 Compare October 14, 2019 07:42
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Oct 14, 2019

💚 Build Succeeded

@dgieselaar dgieselaar force-pushed the integrate-codesearch-ui branch from e416837 to 0251dc9 Compare October 14, 2019 18:16
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Oct 14, 2019

💔 Build Failed

@dgieselaar
Copy link
Copy Markdown
Contributor Author

dgieselaar commented Oct 14, 2019

Currently blocked(-ish) by:

  • highlighting. current highlight functionality is intended for search results rather than APM's usage (line highlighting). Code will extend CodeBlock to support this as well.
  • imports should point to code/public, but importing that file currently leads to the Code application taking over APM's routing.

@dgieselaar
Copy link
Copy Markdown
Contributor Author

dgieselaar commented Oct 15, 2019

@formgeist Should I add spacing between the library stackframes?

image

to

image

@dgieselaar dgieselaar force-pushed the integrate-codesearch-ui branch from 0251dc9 to 7cdf6f2 Compare October 15, 2019 10:14
@formgeist
Copy link
Copy Markdown
Contributor

formgeist commented Oct 15, 2019

@dgieselaar Yes, that looks good

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Oct 15, 2019

💔 Build Failed

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Oct 15, 2019

💚 Build Succeeded

@rylnd rylnd mentioned this pull request Oct 15, 2019
Merged
7 tasks
@dgieselaar dgieselaar force-pushed the integrate-codesearch-ui branch from 9701731 to e56cd73 Compare October 17, 2019 07:36
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Oct 17, 2019

💔 Build Failed

@dgieselaar dgieselaar force-pushed the integrate-codesearch-ui branch from e56cd73 to f201021 Compare October 17, 2019 09:05
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Oct 17, 2019

💚 Build Succeeded

@smith smith added the blocked label Oct 22, 2019
@dgieselaar dgieselaar closed this Oct 25, 2019
@dgieselaar dgieselaar deleted the integrate-codesearch-ui branch October 25, 2019 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rylnd rylnd rylnd left review comments

+1 more reviewer

@mw-ding mw-ding mw-ding left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

blocked release_note:enhancement v7.6.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[APM] Convert APM code preview component to code snippet component from Code Search

8 participants

@dgieselaar @sorenlouv @daveyholler @elasticmachine @formgeist @rylnd @mw-ding @smith