[EuiCodeBlock] Fix line numbers appearing in copied content #6824
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Line numbers must be rendered via CSS pseudo element and not text nodes in order for it not to be copied
- requires giving `EuiCodeBlockAnnotation` a bit more prop flexibility
cee-chen
commented
Jun 1, 2023
Comment on lines
34
to
56
| describe('EuiCodeBlock copy UX', () => { | ||
| beforeEach(() => { | ||
| // Clipboard permissions are required for copy behavior to work in non-Electron browsers | ||
| // @see https://github.com/cypress-io/cypress-example-recipes/blob/182400395817a14f0c7f18889b2fcc6b4d189434/examples/testing-dom__clipboard/cypress/e2e/permissions-spec.cy.js#L37 | ||
| cy.wrap( | ||
| Cypress.automation('remote:debugger:protocol', { | ||
| command: 'Browser.grantPermissions', | ||
| params: { | ||
| permissions: ['clipboardReadWrite', 'clipboardSanitizedWrite'], | ||
| origin: window.location.origin, | ||
| }, | ||
| }) | ||
| ); | ||
| }); | ||
|
|
||
| const assertClipboardContentEquals = (expectedText: string) => { | ||
| cy.window() | ||
| .its('navigator.clipboard') | ||
| .invoke('readText') | ||
| .then((clipboard) => { | ||
| expect(clipboard).equals(expectedText); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
This was a fun rabbit hole to go down - turns out it's much more annoying to assert on copy/clipboard content than I thought.
I went down a separate rabbit hole for RTL to try and get a paste test with that working, but it turns out we're missing several requirements on that front:
@testing-library/user-eventsneeds to be at 14+ (for.setup()API)jestneeds to be at 26+ in order for JSDOM to be at 16+ - otherwise the.getSelectionAPI errors
Since upgrading Jest is a whole nother fun bucket of shenanigans (#6813) I opted for Cypress with these tests for now, but may spike out converting these tests down the road when we upgrade Jest.
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6824/ |
3 tasks
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6824/ |
tkajtoch
reviewed
Jun 2, 2023
| // Note: This must use a CSS pseudo element to print the line number | ||
| // to prevent line numbers from being copied as text | ||
| &::before { | ||
| content: attr(data-line-number); |
There was a problem hiding this comment.
I love this solution for its simplicity!
tkajtoch
approved these changes
Jun 2, 2023
This reverts commit fac3385.
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6824/ |
cee-chen
added a commit
to elastic/kibana
that referenced
this pull request
Jun 3, 2023
This is a backport EUI upgrade to Kibana v8.8.1 containing an `EuiCodeBlock` bugfix requested by the Observability team: #158644 (comment) ## [`77.1.5`](https://github.com/elastic/eui/tree/v77.1.5) **Bug fixes** - Fixed `EuiCodeBlock` potentially incorrectly ignoring lines ending with a question mark when using the Copy button. ([#6794](elastic/eui#6794)) - Fixed `EuiCodeBlock` to not include line numbers when copying content ([#6824](elastic/eui#6824)) - Fixed the expanded row animation on `EuiBasicTable` causing cross-browser Safari issues ([#6826](elastic/eui#6826)) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
cee-chen
added a commit
to elastic/kibana
that referenced
this pull request
Jun 6, 2023
## Summary `eui@81.2.0` ⏩ `eui@81.3.0` ✨ Highlights: - fixes #158644 - Adds a new Timeline icon for use within `EuiMarkdownEditor` (cc @kqualters-elastic) - Expandable rows used within `EuiBasicTable` and `EuiInMemoryTable` now have a faster and snappier expand animation ___ ## [`81.3.0`](https://github.com/elastic/eui/tree/v81.3.0) - Added `timelineWithArrow` glyph to `EuiIcon` ([#6822](elastic/eui#6822)) **Bug fixes** - Fixed `EuiCodeBlock` potentially incorrectly ignoring lines ending with a question mark when using the Copy button. ([#6794](elastic/eui#6794)) - Fixed `EuiCodeBlock` to not include line numbers when copying content ([#6824](elastic/eui#6824)) - Fixed the expanded row animation on `EuiBasicTable` causing cross-browser Safari issues ([#6826](elastic/eui#6826))
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
closes #6818
I broke this behavior in #6580 (annotation feature) by switching from rendering the line numbers as
::beforepseudo elements to actual text node. I've reverted the rendering to use pseudo elements, and cleaned up line number classNames slightly to better reflect the intended DOM structure (see individual commits).I've additionally added new Cypress tests to ensure copy functionality doesn't regress in the future.
QA
General checklist
jest andcypress tests