[SharedUX] SCSS migration of kibana_react plugin#216450
Conversation
paulinashakirova
added
release_note:skip
|
Pinging @elastic/appex-sharedux (Team:SharedUX) |
| const kbnMarkdownAlphaLightShade = `rgba(${euiTheme.colors.fullShade}, .15)`; | ||
| // Reverse grayscale for opposite of theme | ||
| const kbnMarkdownAlphaLightestShadeReversed = `rgba(${euiTheme.colors.emptyShade}, .05)`; | ||
| const kbnMarkdownAlphaLightShadeReversed = `rgba(${euiTheme.colors.emptyShade}, .15)`; |
There was a problem hiding this comment.
using "fullShade" "emptyShade" is said to be deprecated, but since it is a legacy code which was reverted for a specific purpose I decided to go with these variables to stick closer to the original.
| td, | ||
| th { | ||
| padding: 0; | ||
| } |
There was a problem hiding this comment.
This was showing in dev tools that
The display: table-header-group property prevents padding from having an effect.
Try setting display to something other than table-header-group.
So it had no effect anyways.
| // 8. Code blocks | ||
|
|
||
| export const markdownStyles = (isReversed: boolean) => { | ||
| const { euiTheme } = useEuiTheme(); |
There was a problem hiding this comment.
I know it seems this works, but could we instead pass the value of euiTheme from an invocation of useEuiTheme from a component.
| const markdownClasses = classNames('kbnMarkdown__body', { | ||
| 'kbnMarkdown__body--reversed': isBackgroundInverted(panelBackgroundColor), | ||
| 'kbnMarkdown__body--reversed': isReversed, | ||
| }); |
There was a problem hiding this comment.
the markdownClasses declaration can be moved into the markdown component; we already set the kbnMarkdown__body class within our component, also since we already receive the isReversed prop within the component, we can also set the kbnMarkdown__body--reversed class in there, what's more I don't think we actually need to explicitly pass it since our new styles actually have no reference to it, however we should confirm that this classname is not being arbitrarily target from elsewhere.
There was a problem hiding this comment.
our new styles actually have no reference to it,
Yes, and this was my research as well, but then...
this classname is not being arbitrarily target from elsewhere.
I got worried that maybe this is something that is custom targeted from outside.
My research showed that it was used before the migration along with other visualizations, but when they were migrated - this was removed.
Since it is the legacy code - I didn't feel confident removing because of the reason it was brought back in the first place...
@eokoneyo Is there a way to check for custom implementation from outside?
There was a problem hiding this comment.
Since we aren't too sure, I suppose we can keep it around, however I think we can should contain the application of the classname within the markdown component.
eokoneyo
left a comment
eokoneyo
left a comment
There was a problem hiding this comment.
Thanks for making these changes!
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Page load bundle
History
|
## Summary This PR is a part of SCSS migration of SharedUX team code. Here is a [meta](elastic/kibana-team#1417) issue for it.
7 participants
Summary
This PR is a part of SCSS migration of SharedUX team code.
Here is a meta issue for it.