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

Text styles #683

Merged
merged 9 commits into from
Apr 18, 2018
Merged

Text styles #683

merged 9 commits into from
Apr 18, 2018

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Apr 17, 2018

Change some default text styles

  • Set EuiText grow=true as default (fixes Unify max-width rules for titles, text, and form rows #669)
  • Styled dl's inside EuiText
  • Set a min-width of 75% when grow is false - This way it's at least 2/3 the width of the page which will visually look better and so the overall width should be handled by the page itself.
  • Fixed code, pre blocks inside EuiText
  • Fixed ghost color which was being set to a dark gray from the makeHighContrastColor function.

Added to the reset file

There was an issue where input, textarea, select, and buttons were getting the user agent font family instead of inheriting from html. Adding them to the reset file fixes browser defaults from overriding font families.

Before

screen shot 2018-04-17 at 17 29 07 pm

screen shot 2018-04-17 at 17 29 30 pm

After

screen shot 2018-04-17 at 17 30 14 pm

Added support for text alignment (fixes #647)

  • Added EuiTextAlign component
  • Implemented in the same way as EuiTextColor

screen shot 2018-04-17 at 17 27 10 pm

Docs

  • Moved toComponent button into GuidePage

@cchaos cchaos requested review from cjcenizal and snide April 17, 2018 21:55
Copy link
Contributor

@cjcenizal cjcenizal 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 making these changes! I'm especially stoked to see EuiTextAlign finally making it in.

When you update CHANGELOG.md, could you please move the 0.0.40 entry about EuiText under "Breaking changes"? And then could you add a note to that bullet about how it's undone by this change, similar to the style of the deprecation notice in 0.0.19? This way anybody reading the change log will come across that bullet and feel comfy skipping on ahead to this release without worrying about the breaking change.

<p>You can also pass alignment to <EuiCode>EuiText</EuiCode> directly with a prop</p>
</EuiText>
<EuiText textAlign="center" color="secondary">
<p>And in conjuction with coloring.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Small typo: "conjunction"

@@ -98,13 +110,16 @@
list-style: decimal;
}

dt {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add this to the EuiText example?

Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

👍

cchaos added 6 commits April 18, 2018 15:23
- Styled `dl` inside EuiText
- Set a min-width of 75% when grow is false
- Moved toComponent button into GuidePage (docs)
Fixes browser defaults overriding font families for input, textarea, select, button
- Added EuiTextAlign
- Implementing in the same way as EuiTextColor
- Added <dl> to example
- fixed typo
@cchaos
Copy link
Contributor Author

cchaos commented Apr 18, 2018

@cjcenizal I'm confused by your comment about the changelog. I see the bullet:

- Tweaked sizing, weights, color, line-heights, and 
   added more levels to `EuiTitle` and `EuiText` 
   ([#627](https://github.com/elastic/eui/pull/627))

under 0.0.40 but I don't know how this is a breaking change and nor is it completely undone in this PR. The only thing this PR changes from that is making the grow prop true by default which is actually a bullet in 0.0.41:

- Added `grow` prop to `EuiText` ([#662](https://github.com/elastic/eui/pull/662))

@cjcenizal
Copy link
Contributor

cjcenizal commented Apr 18, 2018

@cchaos I would consider the original change to apply max-width to EuiText to be a breaking change, because it has far-reaching impacts on layouts that have been built with assumptions about the width of EuiText. So maybe instead of moving the 0.0.40 bullet, we could add a new one under breaking changes under 0.0.40:

**Breaking changes**

- **Note: This breaking change is reversed in 0.0.43.** Added a default `max-width` to `EuiText`. ([#627](https://github.com/elastic/eui/pull/627))

My addition of the grow prop retained this breaking change, so I don't think it needs to be changed or cross-referenced. I just think we need a note for 0.0.43:

- Removed the default `max-width` from `EuiText`. This can still be applied by setting `grow={false}`. ([#683](https://github.com/elastic/eui/pull/683))

@cchaos
Copy link
Contributor Author

cchaos commented Apr 18, 2018

That works thank you.

@cchaos cchaos merged commit afab8fc into elastic:master Apr 18, 2018
@cchaos cchaos deleted the text-styles branch April 18, 2018 20:15
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 this pull request may close these issues.

Unify max-width rules for titles, text, and form rows Add align prop to <EuiText>
3 participants