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

[DRAFT] Consolidate EuiText and EuiMarkdownFormat styling #4145

Closed
snide opened this issue Oct 15, 2020 · 2 comments · Fixed by #4663
Closed

[DRAFT] Consolidate EuiText and EuiMarkdownFormat styling #4145

snide opened this issue Oct 15, 2020 · 2 comments · Fixed by #4663
Assignees

Comments

@snide
Copy link
Contributor

snide commented Oct 15, 2020

This is a draft document meant to start discussion

Problem

  • EuiText and EuiMarkdownFormat are very closely related, but currently separate. Furthermore, Kibana utilizes its own markdown formatting which differs between the EUI components as well.
  • EUI components often break when included within EuiText, which we technical suggest not doing, but has proved to be needed over the years. As an example, giving a <p> a margin for everything within EuiText can often break a <p> within another EUI component that has very specific sizing needs.

Requirements

Boiled down, we have the following top level needs if we were to share a component for the various use-cases:

  • At a sass mixin / component prop level, we need to optionally support em percentage styling in addition to defaults that are derived from $euiSize variables. This is because markdown is used heavily in dynamic, user-created spaces like Canvas, which will require a backwards compatibility layer enforced through a parent font-sizing value of a parent container.
  • Weed to support modifier based selector sizing for the entire block. This should be supported by passing a size prop onto the component similar to how EuiText works today. This would effect the size of the variables used.
  • Coloring needs to optionally be handled by opacity in addition to defaults derived from $euiColor variables. Again, needed for systems where the background color is different like canvas. This could be alternatively handled without opacity, and derived from a single font-color, but this likely wouldn't be achievable with CSS alone. The reason not to just use transparency for everything is that it can contribute to slowdowns in render painting. Usually we only need this alternative method in outliers where more targeted user-control exists.
  • The full syntax of Github flavored markdown needs to be provided. This means things like table alignment needs to account for naked dom selectors beyond what we currently support in EuiText.
  • Needs to account for selectored HTML coming from other components to co-exist with naked HTML. Most of our attempts at making EuiText magical have assuming that the child content is primarily naked HTML. There are pretty good times though when we want to include EuiComponents inside EuiText and we need to make sure that the margins work correctly. If possible, this should be handled as magically as EuiText normally operates, with some clever CSS beyond our current > * methods.

Relationship between EuiText and EuiMarkdownFormat

  • EuiText should handle the css styling of naked HTML within an .euiText selector.
  • EuiMarkdownFormat should transform markdown syntax into HTML, and include EuiText as the styling mechanism for that HTML.

Pseduo-code sass ideas

At the sass level we probably should have something like:

@mixin euiTextStyle($sizingMethod, $colorMethod, $fontSizeScale, $reverseThemeLayout) {}
  • sizingMethod accepts em or pixel as options, which then cascades to the types of units used through the document.
  • colorMethod accepts solid or translucent as options, which then cascades to whether to use the flat default coloring, or use transparency values for the various color settings.
  • reverseColorsFromTheme accepts true / false. This is needed to handle instance where the background might be flipped against what it usually is.
  • fontSizeScale takes a number value like .75 or 1.5 and adjusts the font sizing and margins accordingly, using em or pixel values as defined by sizingMethod. For the pixel method, it would use mutlipliers against the $euiFontSize base.
.euiText {
  @include euiTextStyle('pixel', 'solid', 1);
}

.euiText--small {
  @include euiTextStyle('pixel', 'solid', .75);
}

.euiText--emSmall {
  @include euiTextStyle('em', 'translucent', .75);
}

Pseudo-component ideas

On the React side we might see something like:

<EuiText size="small" />

// Would just pass the values down to an inner EuiText component
<EuiMarkdownFormat size="small" />
<EuiText size="small" sizingMethod="em" reverseColorsFromTheme colorMethod="translucent" />

// Which would work the same with EuiMarkdownFormat
<EuiMarkdownFormat size="small" sizingMethod="em" reverseColorsFromTheme colorMethod="translucent" />

Issues to solve

This method, since it's Sass heavy, will duplicate a lot of boilerplate selectors as written above. We might want to look at a more overwrite / cascade method to manage the compile size of the css.

@snide snide changed the title Consolidate EuiText and EuiMarkdownFormat styling [DRAFT] Consolidate EuiText and EuiMarkdownFormat styling Oct 15, 2020
@cchaos
Copy link
Contributor

cchaos commented Oct 15, 2020

One other thing to consider is how our baselines line up with our 4px layout grid.

The Amsterdam (@hbharding) team has been discussing some line-height issues we're running into with the smaller Amsterdam font-size and that a 1.5rem -> 21px line-height no longer lands on the layout grid. Testing a 14px font size with a 24px line-height in paragraphs, however, is not an ideal line-height. So I, unfortunately, don't have any good suggestions.

Perhaps, we just ensure that our titles which are what tends to sit horizontally next to other components do always line-up with the 4px grid no matter if they use EuiTitle or headings within EuiText.

@cchaos
Copy link
Contributor

cchaos commented Dec 2, 2020

I would also like to add that any element that gets bottom margin applied should be caveated with :not(:last-child). I continue to run into issues where I have text inside of a panel but there's extra space at the bottom causing the overall padding to be off. Example:

Screen Shot 2020-12-02 at 12 29 55 PM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants