Review updates #10
Review updates #10
Conversation
There was a problem hiding this comment.
Visually the main differences are:
Readonly
Since we allow consumers to pick a color to render the expression in, if it's not clickable we're totally ignoring it and forcing the text color. We shouldn't do this, but align the visual more to how the normal expression works which just removes the border if it's not clickable. So in column layout we remove the background color if not clickbale.
Invalid
We don't need to restrict isInvalid to only the column style layout. The inline version can be too. So I've update the component to render the icon for both.
Active
This is where I sort of took a liberty. I felt the 2px border all the way around was a bit distracting because of how pronounced it was. My thought was, why don't we just keep with the bottom-border concept, and simply add that. I think it looks kinda cool because it's also almost like a shadow.
Focus & hover
I think it's important to keep the coloring from the passed color prop to change the color of the background when in focus. I also added a text underline for both focus and hover states that align to what we do with buttons.
| @@ -103,7 +101,7 @@ export default () => { | |||
| }; | |||
|
|
|||
| const renderPopover1 = () => ( | |||
| <div> | |||
| <div style={{ width: 300 }}> | |||
There was a problem hiding this comment.
These just makes sure the popover doesn't change size on selection change.
| /> | ||
| <EuiSpacer /> | ||
| <EuiTitle size="xxs"> | ||
| <h3>Description width at 50px</h3> |
There was a problem hiding this comment.
This example now shows a more appropriate sized for the word "JOIN"
| @@ -7,11 +7,11 @@ | |||
| @include euiFontSizeS; | |||
| @include euiCodeFont; | |||
|
|
|||
| border-bottom: 2px solid transparent; | |||
| border-bottom: $euiBorderWidthThick solid transparent; | |||
There was a problem hiding this comment.
We actually have a variable for this now
| border-color: transparent; | ||
| // Ensures there's no flash of the dashed style before turning solid for the active state | ||
| border-bottom-style: solid; |
There was a problem hiding this comment.
Keeps the border to only the bottom
| .euiPopover__anchor { | ||
| width: 100%; | ||
| } |
There was a problem hiding this comment.
Don't forget that you can't set external component-specific style like this. It will affect all usages. EuiPopover actually has a display="block" prop to help with this which I added to your examples.
| */ | ||
| descriptionWidth?: number; | ||
| descriptionWidth?: number | string; |
There was a problem hiding this comment.
This allows the consumer to decide how they want to handle the sizing unit. Whether by pixel or percentage or some other unit like vw. Keeps it really flexible by simply passing any type that style.width accepts.
| let colorClass; | ||
| if (display === 'columns' && isInvalid) { | ||
| colorClass = colorToClassNameMap.danger; | ||
| } else { | ||
| colorClass = colorToClassNameMap[color]; | ||
| } | ||
| const calculatedColor = isInvalid ? 'danger' : color; |
There was a problem hiding this comment.
Allows for all display types to be invalid
| const descriptionStyle = descriptionProps && descriptionProps.style; | ||
| const customWidth = | ||
| display === 'columns' && descriptionWidth | ||
| ? { | ||
| flexBasis: `${descriptionWidth}%`, | ||
| flexBasis: descriptionWidth, | ||
| ...descriptionStyle, | ||
| } | ||
| : undefined; |
There was a problem hiding this comment.
Need to also account for the fact that style is a property of descriptionProps. If a consumer passed descriptionProps.style it would have overridden the customWidth style.
| </span> | ||
| {invalidIcon} |
There was a problem hiding this comment.
Simplified the internals now that all displays can be invalid.
| <EuiIcon | ||
| className="euiExpression__icon" |
There was a problem hiding this comment.
It's usually easier to just pass a class directly to the icon instead of creating a wrapper.
|
One other thing I would do, is create a new example section specifically for the |
|
There also still seems to be a bit of a truncation issue. It might still be nice for consumers to be able to adjust this with a simple prop. Similar to how EuiCodeEditor handles
|
I'll comment my thoughts/changes in the diff.