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.

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

[EuiCodeBlock] Replace highlight.js with prism.js via refractor #4638

Merged
merged 32 commits into from
May 4, 2021

Conversation

thompsongl
Copy link
Contributor

@thompsongl thompsongl commented Mar 11, 2021

Summary

Closes #3947 (and replaces #4400)

Replaces highlight.js with prism.js via refractor for code syntax highlighting.

This is currently a 1:1 replacement with no additional features (e.g., code block virtualization).
Current bundle size reduction: ~1.5MB (~1.1MB minified) 🎉

Affects:

  • EuiCode
  • EuiCodeBlock
  • EuiMarkdownEditor (plugins)
  • EuiMarkdownFormatter (plugins)

New:

  • remark-prismjs plugin for EuiMarkdown* (replaces 3rd-party remark-highlight.js)

Breaking change(s):

  • Content passed to EuiCode and EuiCodeBlock must be string-based for proper syntax highlighting
    • We can still render the input, but prismjs cannot parse it, so it will not be highlighted
    • We need to accept non-string content for EuiMarkdown[Editor | Formatter] because input is pre-parsed
  • No syntax language auto-detection (Not actually breaking)
  • Different language names/aliases from highlight.js
    • text does not exist
    • sh is not an alias for bash
    • js will not highlight jsx-specific tokens
    • We can patch some of these discrepancies, but is it worth it?
  • See further explanation in this comment

To do:

Checklist

  • Check against all themes for compatibility in both light and dark modes

- [ ] Checked in mobile

- [ ] 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_4638/

@kibanamachine
Copy link

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

@thompsongl
Copy link
Contributor Author

Research line numbers

Certainly possible. Will need to update our nodeToHtml method to group lines based on EOL AST nodes ({type: "text", value: "↵ "} and manage highlighting per line. All HTML is fully under our control, so this is just a matter of defining preferential styles and customization.
Relatedly, my guess is that we will need to do per-line highlighting after EOL grouping to build virtualization, also.

@thompsongl
Copy link
Contributor Author

thompsongl commented Mar 18, 2021

Research virtualizing code blocks (react-window)

Certainly possible.
Confirmed my guess that we would need per-line grouping and highlighting to enable react-window virtualization.
I'm going to push the prerequisite line grouping work to this branch, but I'd like to make virtualization its own PR after we're set on the fundamental prismjs conversion.

@kibanamachine
Copy link

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

@thompsongl
Copy link
Contributor Author

thompsongl commented Mar 18, 2021

Research reducing initial language load

Yes. We can save up to an additional ~300k (~1.8MB total) by not including the full language set. Instead, we'd load refractor/core (which includes clike, css, js, html, mathml, svg, xml, ssml, atom, rss) and register additional languages, like so:

import refractor from 'refractor/core';
import jsx from 'refractor/lang/jsx';
import sql from 'refractor/lang/sql';

refractor.register(jsx);
refractor.register(sql);

Obviously we'd need to decide on EUI's core set of supported languages.
Allowing consumers to register additional languages (if so decided) would require exposing our internal refractor instance.

@thompsongl
Copy link
Contributor Author

Research loading languages async

Yes, but probably an either/or with "reducing initial language load". react-syntax-highlighter provides a good idea of how we might approach async loading all languages. We're already getting big improvements without optimizing for languages, so I'd also like to take this on separately with more deliberation on async loading vs. limited language set.

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@thompsongl
Copy link
Contributor Author

thompsongl commented Mar 18, 2021

Opening this up for review. Looking for early feedback on:

  • Dark mode colors. Not any worse than what's in production now, but open to design work against this branch
  • Breaking change tolerance. See ^ "Summary" for tentative breaks

@thompsongl thompsongl marked this pull request as ready for review March 18, 2021 22:02
@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@cchaos
Copy link
Contributor

cchaos commented Mar 29, 2021

Obviously we'd need to decide on EUI's core set of supported languages.
Allowing consumers to register additional languages (if so decided) would require exposing our internal refractor instance.

I'm fine with this so long as we document this very well.

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Fantastic conversion! I was happy to see how straight forward the markdown changes are, in particular. Couple notes on windows line breaks but everything else LGTM!

src/components/code/_code_block.tsx Outdated Show resolved Hide resolved
src/components/code/_code_block.tsx Show resolved Hide resolved
src/components/code/_code_block.tsx Outdated Show resolved Hide resolved
@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Changes LGTM! Pulled & tested locally

@kibanamachine
Copy link

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

@thompsongl thompsongl merged commit 7aeb30f into elastic:master May 4, 2021
cchaos added a commit that referenced this pull request May 4, 2021
* replace highlight.js with prism.js via refractor

* merge fix: file renamed

* group lines

* update tests

* i18n block

* remove regex lookbehind not supported in safari

* CL

* js -> jsx

* token color parity

* update line grouping method

* use jsx language in docs

* reduce newlines

* docs

* docs

* headings

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

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>

* playground updates

* os newlines

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
tkajtoch added a commit to tkajtoch/eui that referenced this pull request May 23, 2023
…a single one when using the Copy button

This change was first introduced in 7aeb30f ([EuiCodeBlock] Replace highlight.js with prism.js via refractor (elastic#4638))
but doesn't seem to be a business or product decision.

We decided to remove it to make the copied content match what's displayed on screen.
tkajtoch added a commit to tkajtoch/eui that referenced this pull request Jun 1, 2023
…a single one when using the Copy button

This change was first introduced in 7aeb30f ([EuiCodeBlock] Replace highlight.js with prism.js via refractor (elastic#4638))
but doesn't seem to be a business or product decision.

We decided to remove it to make the copied content match what's displayed on screen.
tkajtoch added a commit to tkajtoch/eui that referenced this pull request Jun 2, 2023
…a single one when using the Copy button

This change was first introduced in 7aeb30f ([EuiCodeBlock] Replace highlight.js with prism.js via refractor (elastic#4638))
but doesn't seem to be a business or product decision.

We decided to remove it to make the copied content match what's displayed on screen.
tkajtoch added a commit that referenced this pull request Jun 2, 2023
…character (#6794)

* fix(EuiCodeBlock): fix code block copy button not including the last character if it's a question mark

* chore: add changelog file

* fix(EuiCodeBlock): Remove reducing two or more consecutive empty lines to a single one when using the Copy button

This change was first introduced in 7aeb30f ([EuiCodeBlock] Replace highlight.js with prism.js via refractor (#4638))
but doesn't seem to be a business or product decision.

We decided to remove it to make the copied content match what's displayed on screen.

* Revert "fix(EuiCodeBlock): Remove reducing two or more consecutive empty lines to a single one when using the Copy button"

This reverts commit 8a90215c81432514493b1bb29ecd314d6792a05d.

* docs: add clarification comment why \n\n replace is needed

* Update regression test

* changelog

---------

Co-authored-by: Constance Chen <constance.chen.3@gmail.com>
Co-authored-by: Cee Chen <549407+cee-chen@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[EuiCodeBlock] Replace highlight.js as the engine
4 participants