[EuiCodeBlock] Add line numbers#4993
Conversation
|
@miukimiu Would you please take a look at this from a design perspective before I open for full review? |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_4993/ |
|
Weird thing in Safari. Copying the selection as shown in the image does not actually copy the line numbers; it just appears as though they are selected. Have not been able to prevent apparent selection.
|
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_4993/ |
|
I'm pretty sure that would be the behavior that we want (not copying the line-numbers). So perhaps there's a setting that is actually only working in Safari? You can also pass |
Yes, I think I worded that poorly. All browsers do not copy the line numbers, which is good. Safari is the only browser that makes it appear as though the line numbers are selected. Other browsers:
This is there already but has no effect on what Safari does. |
|
Looks like it's because it's technically still highlighting the generic row. What you'll need to do is apply the setting the whole wrapper, then add it back on with |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_4993/ |
|
Opening this up for review, but very much open to design changes 🙇♂️ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_4993/ |
elizabetdev
left a comment
There was a problem hiding this comment.
This is a great feature, and it's looking good! 😍
I found a few issues.
When we have a non-virtualized code block and a vertical scrollbar appears the line numbers get out of the container:
I used the following example in eui/src-docs/src/views/code/code_block.js: https://gist.github.com/miukimiu/4c01830ecaa0dd2e442780ba16c98a5b.
Screen size 1440 x 1024.
Another issue is when we have a whiteSpace="pre". All the content scrolls and the numbers overlap the text.
Ideally, the horizontal scrollbar should start after the line numbers:
Screen.Recording.2021-09-09.at.03.18.PM.mov
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_4993/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_4993/ |
elizabetdev
left a comment
There was a problem hiding this comment.
Looks good @thompsongl! 🎉
Tested again in Chrome, Safari, Edge, and Firefox.
cee-chen
left a comment
There was a problem hiding this comment.
Sorry for the millions of questions, it's my first time looking at this component so I have a lot of really annoying q's/comments 😅 QA itself looks great and amazing, so mostly just very nitty stuff from me
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_4993/ |
| properties: { | ||
| style: { marginLeft: width + 8 }, // 8px is $euiSizeS | ||
| className: ['euiCodeBlock__line__text'], | ||
| style: { marginLeft: width + $euiSizeS }, |
There was a problem hiding this comment.
TIL variables can start with a special character! Awesome 🤯
cee-chen
left a comment
There was a problem hiding this comment.
Changes and a quick QA of the docs look great! 🎉 My above unit testing comment is 100% optional
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_4993/ |
Summary
Closes #3140 by adding an option to render line numbers in EuiCodeBlock. Works with virtualization and fullscreen, and does not impact copy functionality.
Checklist
fec4925