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

Fix filled EuiButton in EuiControlBar #2781

Merged
merged 5 commits into from
Jan 22, 2020

Conversation

andreadelrio
Copy link
Contributor

@andreadelrio andreadelrio commented Jan 21, 2020

Summary

Before

image

Now

image

And here's how filled EuiButtons look in all four themes

control_bar_themes1

Fixes #2768

Checklist

  • Check against all themes for compatibility in both light and dark modes
    - [ ] Checked in mobile
    - [ ] Checked in IE11 and Firefox
    - [ ] Props have proper autodocs
    - [ ] Added documentation examples
    - [ ] Added or updated jest tests
    - [ ] Checked for breaking changes and labeled appropriately
    - [ ] Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

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.

Can you include an example of the button in one of the docs examples. Stylistic feedback, so I'll let @ryankeairns chime in, but I don't think you need the border in the filled version. That way it's closer to the feel of the other buttons on the page.

image

@ryankeairns
Copy link
Contributor

+1 for dropping the border on the filled versions.

@andreadelrio
Copy link
Contributor Author

I removed the border from the fill button and I added one of them to our examples.

It looks like this

image

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.

Small CL suggestion for clarity. Otherwise LGTM. Thanks!

CHANGELOG.md Outdated
@@ -11,6 +11,7 @@
**Bug fixes**

- Increased column width on `EuiTableHeaderCellCheckbox` to prevent `EuiCheckbox`'s focus ring from getting clipped in `EuiBasicTable` ([#2770](https://github.com/elastic/eui/pull/2770))
- Fixed filled `EuiButton`s in `EuiControlBar` ([#2781](https://github.com/elastic/eui/pull/2781))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Fixed filled `EuiButton`s in `EuiControlBar` ([#2781](https://github.com/elastic/eui/pull/2781))
- Fixed the display of `EuiButton` within `EuiControlBar` when `fill={true}` to be more consistent with other buttons ([#2781](https://github.com/elastic/eui/pull/2781))

@ryankeairns
Copy link
Contributor

ryankeairns commented Jan 22, 2020

I pulled this up locally and quickly got sidetracked into other conversations, but I'm curious what colors this is using for buttons. They look lighter than our standard dark-mode button colors 🤔

The borderless approach looks great!

@ryankeairns
Copy link
Contributor

ryankeairns commented Jan 22, 2020

Looked a little more and I think my comment might be opening a rabbit hole that can be addressed at a later time. The control bar background is darker with more vibrant button colors in dark mode, while we use a lighter background and lighter button colors in light mode. I wonder about that latter combination, but could punt on that for now.

@snide
Copy link
Contributor

snide commented Jan 22, 2020

I noticed that too. I think we should address that separately. To be honest, this is something better solved by a JS theming layer, otherwise we're writing waaaaay too much sass in inception like ways. A little is fine, but if we know (which we do) that this will primarily be for the primary / secondary fill buttons, I think we should be OK letting this go till the theming layer is more robust (for this theme within a theme stuff)

@andreadelrio andreadelrio merged commit 02de242 into elastic:master Jan 22, 2020
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.

EuiControlBar doesn't display correct colors in filled button
3 participants