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.

Sign up for GitHub

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

Jump to bottom

[EuiCodeBlock] Virtualized option #4952

Merged
merged 16 commits into from
Jul 26, 2021
Merged

[EuiCodeBlock] Virtualized option #4952

merged 16 commits into from
Jul 26, 2021

Conversation

thompsongl
Copy link
Contributor

@thompsongl thompsongl commented Jul 15, 2021 •
edited
Loading

Summary

Closes #1208 by adding an option to virtualize line rendering using react-window.

  • Virtualization is opt-in using the isVirtualized prop. Setting overflowHeight is required with this configuration.
  • overflowHeight now accepts a percentage value in string form. Consumers need to ensure the parent has calculable height.

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 the any docs examples
  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

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

thompsongl
thompsongl commented Jul 15, 2021
View reviewed changes
src-docs/src/views/code/code_example.js
@@ -37,7 +45,7 @@ export const CodeExample = {
<p>
The <strong>EuiCode</strong> and <strong>EuiCodeBlock</strong>{' '}
components support{' '}
<EuiLink external href="https://github.com/wooorm/refractor#syntaxes">
<EuiLink external href="https://prismjs.com/#supported-languages">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No language changes here. Just being consistent on links to prismjs vs. refractor

@thompsongl thompsongl marked this pull request as ready for review July 15, 2021 21:10
@kibanamachine
Copy link

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

myasonik
myasonik approved these changes Jul 16, 2021
View reviewed changes
Copy link
Contributor

@myasonik myasonik left a comment

Choose a reason for hiding this comment

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

Tested in VO and virtualization works with no issues!

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

cchaos
cchaos reviewed Jul 16, 2021
View reviewed changes
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

The new example LGTM 👍 . I had one question around a particular use-case and a quick comment suggestion.

@kibanamachine
Copy link

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

@thompsongl thompsongl requested a review from cchaos July 20, 2021 15:24
@kibanamachine
Copy link

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

1 similar comment
@kibanamachine
Copy link

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

@cchaos
Copy link
Contributor

cchaos commented Jul 22, 2021

One thing I noticed, is that the virtualized examples all have isCopyable, but the button isn't showing up.

@thompsongl
Copy link
Contributor Author

One thing I noticed, is that the virtualized examples all have isCopyable, but the button isn't showing up.

Good catch. It used to work; taking a look.

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@thompsongl
Copy link
Contributor Author

flaky test

jenkins test this

cchaos
cchaos approved these changes Jul 26, 2021
View reviewed changes
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

👍 Thanks for fixing the copy function.

@kibanamachine
Copy link

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

@thompsongl thompsongl merged commit d4b843e into elastic:master Jul 26, 2021
@chandlerprall chandlerprall mentioned this pull request Jul 29, 2021
Merged
@thompsongl thompsongl mentioned this pull request Aug 23, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cchaos cchaos cchaos approved these changes

@myasonik myasonik myasonik approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[EuiCodeBlock] Needs virtualization
4 participants
@thompsongl @kibanamachine @cchaos @myasonik