-
Notifications
You must be signed in to change notification settings - Fork 843
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
EuiCode now consumes EuiCodeBlock and can highlight text #138
Conversation
CHANGELOG.md
Outdated
@@ -1,6 +1,7 @@ | |||
# [`master`](https://github.com/elastic/eui/tree/master) | |||
|
|||
- Changed the hover states of `EuiButtonEmpty` to look more like links. | |||
- Added `inline` and `transparentBackground` props to `EuiCodeBlock`. Made light theme the default. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Team light theme wins! Can I get a link to the PR at the end of this line? Same ask as in the other PR:
- Added `inline` and `transparentBackground` props to `<EuiCodeBlock>`. [(#138)](https://github.com/elastic/eui/pull/138)
- The default `<EuiCodeBlock>` `theme` prop value is now `light`. [(#138)](https://github.com/elastic/eui/pull/138)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I specifically didn't add them because your example docs didn't have them 🗡
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I shall fix that
src/components/code/code_block.js
Outdated
}, | ||
{ | ||
'euiCodeBlock--inline': inline, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Loving this. I think the transparentBackground
default should be smart: if it's not set, can we use inline === true
instead?
That'd be:
{
'euiCodeBlock--inline': inline,
'euiCodeBlock--transparentBackground': typeof transparentBackground === 'boolean'
? transparentBackground
: inline,
}
@bevacqua OK. Ready for review again. |
@@ -1,3 +1,4 @@ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove!
src/components/code/code.js
Outdated
<code | ||
<EuiCodeBlock | ||
transparentBackground={transparentBackground} | ||
language={language} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to mention language
or transparentBackground
explicitly, since they are taken care of by ...rest
. Same goes for the propType
src/components/code/code_block.js
Outdated
<pre className="euiCodeBlock__pre"> | ||
<code | ||
<span className="euiCodeBlock__pre"> | ||
<span |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we keep <pre>
and <code>
because semantics?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
React spits out errors if <pre>
tags live within text content. I guess I can keep code though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🎉
Needed this for some concepts I'm working on for Discover. This also defaults the theme to the light coloring, since I seem to be the minority in the dark theming.
EuiCode
now consumesEuiCodeBlock
.EuiCode
now can carry syntax highlighting. Further, the default coloring was changed.For a later PR