Conversation
|
jenkins test this |
|
@snide lint error 17:52:24 > @elastic/eui@0.0.45 lint /app |
cchaos
left a comment
cchaos
left a comment
There was a problem hiding this comment.
Thanks for building these. I just had a couple comments/questions.
| <div className="eui-textCenter"> | ||
| <EuiCode>.eui-textCenter</EuiCode> | ||
| </div> | ||
| <div className="eui-textRight"> |
There was a problem hiding this comment.
Why wouldn't they use EuiTextAlign for this?
There was a problem hiding this comment.
They can. I think for a lot of these simple text treatments through really just some quicky classes are more convenient.
There was a problem hiding this comment.
Example... Here's a common one...
<EuiFlexItem className="eui-textRight" />
Dunno. Just think some of these are so common you don't necessarily want to go through the process of importing a component and writing a couple extra lines of wrappers.
There was a problem hiding this comment.
Yeah I agree that having those will be nice, but I was just concerned about how far we go with utility classes that duplicate the functionality of components.
There was a problem hiding this comment.
Should we just remove the <EuiTextAlign> component in favor of these utility classes?
There was a problem hiding this comment.
I think it's ok to have both. Align gets imported into EuiText so I think we're ok there. Neither are really things that should ever change. I think it's OK to leave it.
I don't think we want many utility classes, and don't think we should take it very far. That's why I wanted to start small.
|
|
||
| <div> | ||
| <EuiIcon type="logoElasticStack" size="xxl" /> | ||
| <EuiCode className="eui-alignTop">.eui-alignTop</EuiCode> |
There was a problem hiding this comment.
I would think you'd want to add these utilities to the icon instead. Especially for the baseline version:
On text
On Icon
|
|
||
| <h4>Display</h4> | ||
|
|
||
| <EuiCode className="eui-block">.eui-block</EuiCode> |
There was a problem hiding this comment.
Should these classes be eui-displayBlock, eui-displayInline, etc?
There was a problem hiding this comment.
I went with speed for some of these. For example, all the text ones don't say... eui--wordWrapBreakAll.
There was a problem hiding this comment.
But I'm indifferent. If we want to be more explicit that's fine as well.
There was a problem hiding this comment.
I'm just worried that "block" is ambiguous if someone is reading scanning the source code.
There was a problem hiding this comment.
Sounds good I'll change it.
snide
force-pushed
the
feature/utility_classes
branch
from
07483af to
5915cc5
Compare
3 participants
Fixes #758
After seeing separate instances of people wanting to deal with text wrapping issues we decided it best to provide some CSS utility classes for common situations. Otherwise we'd need to add lots of optional props to our React components that might not target the individual piece of content (like a heading in a tooltip) that we want.
More discussion can be found in the issue above.
I'm purposefully keeping this list very small to start. For example, keeping out flexs and floats since we already have components that do a decent job at this type of thing.