[EuiCode][EuiCodeBlock] Various cleanups and bug fixes #5379

cee-chen · 2021-11-16T02:05:52Z

Summary

closes #5173
closes #5381
closes #5159

This PR primarily separates EuiCode and EuiCodeBlock from sharing as much unnecessary code/state as it does (since EuiCode is dramatically simpler) and DRYs out the code that they do share into utils.

Along the way however I noticed several other bugs and things to clean up - so I strongly recommend following along by commit. I should hopefully have left enough context and rationale for certain changes/decisions in my commit messages - please let me know if not!

QA

Checklist

Check against all themes for compatibility in both light and dark modes
Checked in mobile
Checked in Chrome, Safari, Edge, and Firefox
Props have proper autodocs and playground toggles
Added documentation
Checked Code Sandbox works for any docs examples
Added or updated jest and cypress tests
Checked for breaking changes and labeled appropriately

⚠️ I don't think this PR contains any breaking changes since EuiCodeBlockImpl should have been an internal implementation only, but let me know if you disagree. I also don't think the CSS class changes on EuiCode is technically a breaking change, from my understanding of how we've handled CSS changes.

Checked for accessibility including keyboard-only and screenreader modes
A changelog entry exists and is marked appropriately

+ write unit tests + organize utils with comment blocks

+ fix external link + remove prop already listed in CommonProps

+ remove prismjs class and language-* class - they didn't seem to be doing anything 🤷 + remove language class - it also wasn't doing anything, and maybe belongs better as a data attribute + add unit tests for language and background props

+ removing any `inline` conditional logic + cleaning up props type/order

…t.tsx

- The export shouldn't have been exported in the first place most likely, and hopefully wasn't being used by any consumers - The testenv I'm less certain about but it looks like we used to `createPortal()` directly at one point and no longer do so/use EuiMaskOverlay, so it shouldn't be needed anymore

- By switching to render instead of html for snapshotting (tbh this seems easier to read in any case) + fix another instance in EuiDraggable that was bogarting this html.d.ts

- As far as I can tell, the prismjs CSS isn't doing anything in FF or Chrome (selections still work fine), and the language class is not doing anything either

kibanamachine · 2021-11-16T03:02:05Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5379/

kibanamachine · 2021-11-16T19:19:46Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5379/

thompsongl · 2021-11-16T19:33:16Z

⚠️ I don't think this PR contains any breaking changes since EuiCodeBlockImpl should have been an internal implementation only, but let me know if you disagree. I also don't think the CSS class changes on EuiCode is technically a breaking change, from my understanding of how we've handled CSS changes.

Agreed on all counts

cchaos

The visual improvements LGTM! I concentrated my review on looking for possible breaks and design code/docs.

src/themes/eui-amsterdam/overrides/_code_block.scss

src/components/code/code.tsx

src/components/code/_variables.scss

src-docs/src/views/code/playground.js

src-docs/src/views/code/code_example.js

src/components/code/_index.scss

+ Update full screen unit test to snapshot to show aria-label/data-test-subj correctly cascading

- the keydown was on the `<code>` element, but it needs to be on an element with `tabindex="0"` which is the `<pre>` element

+ virtualized line height on fontSize="l"

- Give EuiCode its own stylesheet/class name rather than being `euiCodeBlock--inline` - Split out shared color/typography CSS into variable mixins - Split out shared syntax styling CSS into mixin (tokens being in their own separate file is easier to parse also, just IMO) - Adjust EuiCode classNames/snapshots accordingly - rename override file to _code to match component folder

- Add reasonable inputs for lineNumbers and overflowHeight - they take over options/types, but picking these allows users to quickly and easily see the difference in changing these props

+ fix incorrect `color="dark"` prop (not sure where this came from, it was also getting `...rest`ed when it should not have been)

+ fix some funky grammar

- role="presentation" wasn't the correct way to fix the jsx/a11y lint complaint and has actual screen reader ramifications we don't want - After discussing with trevor, the jsx/a11y lint is over-zealous in this case, the end-behavior works better for users with tabIndex and onKeydown, and the rule can be disabled

see elastic#5381 (comment)

- despite the comment which has apparently been incorrect for 4+ years

- Fixing the virtualization bug got me frustrated with working on the component. Various bits of different state and logic were scattered all over 450~ lines and made jumping up and down the file relatively annoying. I decided to split this component up into several different hook/sub-component helpers based on several different complex pieces of functionality: - overflow detection & observers - copy functionality - full screen functionality - virtualization This cleanup also: - gets us to 100% statement coverage (woo) - removes the need for certain lint disable comments (good times with ...rest) - adds/cleans up several code comments TBH, the file is still a little meaty to get through though the organization helps, if we continue to add more functionality to EuiCodeBlock it would be worth considering splitting up the separate sections/helpers into their own files.

…itence - Allows for better line-wrapping of inline code - Also removes `euiCodeTypography()` mixin

cchaos · 2021-11-17T15:02:51Z

Pushed you a commit 1f6b548

kibanamachine · 2021-11-17T15:33:35Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5379/

src/components/text/_text.scss

src-docs/src/views/code/code_example.js

cee-chen · 2021-11-17T17:06:47Z

Changes look super amazing and I LOVE getting rid of that extra span wrapper! Everything feels much cleaner now, thanks Caroline! 🧙

- This should have been added in 4e83aef, but I forgot

kibanamachine · 2021-11-17T17:42:14Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5379/

Co-authored-by: Constance <constancecchen@users.noreply.github.com>

kibanamachine · 2021-11-17T18:51:15Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5379/

kibanamachine · 2021-11-18T21:59:52Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5379/

cchaos

Design and design-code changes LGTM! Thanks for finding and fixing those bugs too.

I looked for any other backlog issues regarding EuiCodeBlock and found #5159. I investigated and I think if we just add white-space: pre; to .euiCodeBlock__line should fix that. It just means that virtualized code blocks cannot respect the whiteSpace prop and always forces topre.

thompsongl · 2021-11-22T16:07:50Z

It just means that virtualized code blocks cannot respect the whiteSpace prop and always forces topre

Could enforce this in TypeScript with ExclusiveUnion

- because react-window virtualization sets the same height for each line, we need to enforce a white-space of `pre` for virtaulization - Fix incorrect type union not actually enforcing `overflowHeight` (isVirtualized needs to be set to `false`), and add unit tests with ts-expect-error for incorrect props - Improve prop docs dev experience slightly, move default comment blocks to to main props interface and have the ExclusiveUnion only handle prop differences

cee-chen · 2021-11-22T22:17:27Z

5ddb91f should fix #5159, and also incidentally fix our type exclusive union not properly enforcing overflowHeight when isVirtualized is set

thompsongl

💟 Thank you!

Ran through the QA steps and otherwise put the examples through the paces. The only discernible difference is perhaps improved scroll updating in the virtualized blocks.

Just the one question

thompsongl · 2021-11-19T15:19:24Z

src/components/code/code.tsx

+  );
+
+  return (
+    <code className={classes} data-code-language={language} {...rest}>


data-code-language is new, right? A debugging helper?

It's new! We previously had language as a CSS class but it wasn't doing anything in terms of styling. I removed the CSS class but figured it would be helpful to leave the current language in as an data attribute for debugging, like you said, and if consumers really needed to hook into it for some reason.

thompsongl · 2021-11-19T15:22:56Z

src/components/code/code_block.tsx

+/**
+ * Virtualization logic
+ */


🤩 this whole refactor

cee-chen · 2021-11-23T00:20:01Z

jenkins test this

kibanamachine · 2021-11-23T00:51:32Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5379/

cchaos

💯 I love tackling that backlog!

I also don't think any of this is a breaking change because I see them all as bug fixes.

cee-chen · 2021-11-23T15:48:00Z

Wahoo! Thanks y'all. Merging!

cee-chen added 8 commits November 15, 2021 14:30

DRY out code used by both inline code & code blocks to utils

19bbc64

+ write unit tests + organize utils with comment blocks

DRY out shared props between inline code & code blocks

6709301

+ fix external link + remove prop already listed in CommonProps

Refactor EuiCode to not use EuiCodeBlockImpl

ed3ad1c

+ remove prismjs class and language-* class - they didn't seem to be doing anything 🤷 + remove language class - it also wasn't doing anything, and maybe belongs better as a data attribute + add unit tests for language and background props

Refactor EuiCodeBlockImpl to EuiCodeBlock

f59f473

+ removing any `inline` conditional logic + cleaning up props type/order

Delete _code_block.test.tsx and move relevant tests to code_block.tes…

27d7278

…t.tsx

Delete unnecessary html.d.ts

50cb9e3

- By switching to render instead of html for snapshotting (tbh this seems easier to read in any case) + fix another instance in EuiDraggable that was bogarting this html.d.ts

Remove unused prismjs and language classes

22d5351

- As far as I can tell, the prismjs CSS isn't doing anything in FF or Chrome (selections still work fine), and the language class is not doing anything either

cee-chen requested a review from thompsongl November 16, 2021 02:05

cee-chen mentioned this pull request Nov 16, 2021

[EuiCodeBlock] Opening virtualized code blocks in full screen mode blanks out the virtualized content #5381

Closed

cee-chen force-pushed the eui-code-refactors branch from 0011105 to efb6b25 Compare November 16, 2021 02:36

cchaos suggested changes Nov 16, 2021

View reviewed changes

cee-chen added 10 commits November 16, 2021 12:29

Fix missing ...rest not spreading to fullscreen/virtualized elements

9bdbd49

+ Update full screen unit test to snapshot to show aria-label/data-test-subj correctly cascading

Fix broken fullscreen escape key

f22b027

- the keydown was on the `<code>` element, but it needs to be on an element with `tabindex="0"` which is the `<pre>` element

Fix fontSize="l" on Amsterdam theme

1f112f1

+ virtualized line height on fontSize="l"

Fix snapshots mounting EuiCodeBlock

9afd3b4

[Docs] Improve EuiCodeBlock playground toggles

82fdd9e

- Add reasonable inputs for lineNumbers and overflowHeight - they take over options/types, but picking these allows users to quickly and easily see the difference in changing these props

[Docs] Convert code source to .tsx

1b57a1d

+ fix incorrect `color="dark"` prop (not sure where this came from, it was also getting `...rest`ed when it should not have been)

Changelog TODOs

3a530ec

+ fix some funky grammar

Fix bug where virtualized code blocks would reset on full screen toggle

4e83aef

see elastic#5381 (comment)

cee-chen force-pushed the eui-code-refactors branch from b2390e9 to 1d6896a Compare November 16, 2021 23:27

Fix full screen mode not actually using either large font or padding

bb4bf79

- despite the comment which has apparently been incorrect for 4+ years

cee-chen force-pushed the eui-code-refactors branch from 1d6896a to 3ec6baf Compare November 16, 2021 23:31

cee-chen added 2 commits November 16, 2021 15:45

Merge branch 'main' into eui-code-refactors

f6c34be

[EuiCode] Reduces wrappers to just <code> and fixed EuiText’s inher…

1f6b548

…itence - Allows for better line-wrapping of inline code - Also removes `euiCodeTypography()` mixin

cee-chen commented Nov 17, 2021

View reviewed changes

src/components/text/_text.scss Outdated Show resolved Hide resolved

cee-chen commented Nov 17, 2021

View reviewed changes

src-docs/src/views/code/code_example.js Show resolved Hide resolved

Add missing changelog

0c21ac4

- This should have been added in 4e83aef, but I forgot

Fix typo

73e97b3

Co-authored-by: Constance <constancecchen@users.noreply.github.com>

Merge branch 'main' into eui-code-refactors

b1568c7

cee-chen requested a review from cchaos November 18, 2021 21:35

cchaos reviewed Nov 20, 2021

View reviewed changes

cee-chen added 2 commits November 22, 2021 13:16

Merge branch 'main' into eui-code-refactors

35f2bf5

thompsongl approved these changes Nov 22, 2021

View reviewed changes

cee-chen requested a review from cchaos November 23, 2021 00:10

cchaos approved these changes Nov 23, 2021

View reviewed changes

cee-chen merged commit 8c603d3 into elastic:main Nov 23, 2021

cee-chen deleted the eui-code-refactors branch November 23, 2021 15:48

thompsongl mentioned this pull request Nov 23, 2021

[EuiCodeBlock] Virtualization doesn't calculate correctly long tokens that break into a new line #5159

Closed

cee-chen mentioned this pull request Nov 24, 2021

[EuiCodeBlock] Fix HTML attributes not being accepted as props #5413

Merged

2 tasks

thompsongl mentioned this pull request Dec 9, 2021

Upgrade EUI to v43.1.1 elastic/kibana#120935

Merged

1 task

cchaos mentioned this pull request Feb 11, 2022

[EuiCode] Fix bug where padding is not being contained in the inline box #5629

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiCode][EuiCodeBlock] Various cleanups and bug fixes #5379

[EuiCode][EuiCodeBlock] Various cleanups and bug fixes #5379

cee-chen commented Nov 16, 2021 •

edited

Loading

kibanamachine commented Nov 16, 2021

kibanamachine commented Nov 16, 2021

thompsongl commented Nov 16, 2021

cchaos left a comment

cchaos commented Nov 17, 2021

kibanamachine commented Nov 17, 2021

cee-chen commented Nov 17, 2021

kibanamachine commented Nov 17, 2021

kibanamachine commented Nov 17, 2021

kibanamachine commented Nov 18, 2021

cchaos left a comment

thompsongl commented Nov 22, 2021

cee-chen commented Nov 22, 2021

thompsongl left a comment

thompsongl Nov 19, 2021

cee-chen Nov 22, 2021

thompsongl Nov 19, 2021

cee-chen commented Nov 23, 2021

kibanamachine commented Nov 23, 2021

cchaos left a comment

cee-chen commented Nov 23, 2021

[EuiCode][EuiCodeBlock] Various cleanups and bug fixes #5379

[EuiCode][EuiCodeBlock] Various cleanups and bug fixes #5379

Conversation

cee-chen commented Nov 16, 2021 • edited Loading

Summary

QA

Checklist

kibanamachine commented Nov 16, 2021

kibanamachine commented Nov 16, 2021

thompsongl commented Nov 16, 2021

cchaos left a comment

Choose a reason for hiding this comment

cchaos commented Nov 17, 2021

kibanamachine commented Nov 17, 2021

cee-chen commented Nov 17, 2021

kibanamachine commented Nov 17, 2021

kibanamachine commented Nov 17, 2021

kibanamachine commented Nov 18, 2021

cchaos left a comment

Choose a reason for hiding this comment

thompsongl commented Nov 22, 2021

cee-chen commented Nov 22, 2021

thompsongl left a comment

Choose a reason for hiding this comment

thompsongl Nov 19, 2021

Choose a reason for hiding this comment

cee-chen Nov 22, 2021

Choose a reason for hiding this comment

thompsongl Nov 19, 2021

Choose a reason for hiding this comment

cee-chen commented Nov 23, 2021

kibanamachine commented Nov 23, 2021

cchaos left a comment

Choose a reason for hiding this comment

cee-chen commented Nov 23, 2021

cee-chen commented Nov 16, 2021 •

edited

Loading