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

Created compressed EuiButtonGroup #2343

Merged
merged 6 commits into from
Sep 18, 2019

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Sep 13, 2019

To match our new compressed form controls

EuiButtonGroup's buttonSize prop now also accepts compressed. Which creates the following.

I also beefed up the tests for EuiButtonGroup that only had one in it.

Checklist

  • Checked in dark mode
  • 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

@cchaos
Copy link
Contributor Author

cchaos commented Sep 13, 2019

The design was based on our Sketch file. But I feel that the selected states aren't pronounced enough, the iconOnly version in particular.

It really gets lost in the complex example:

Screen Shot 2019-09-13 at 16 38 00 PM

I'm open to any suggestions on the design here.

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.

Scanned the code and tested in browser.

Noticed the following hover glitch. I think you might always want to add a title to your example. If you're feeling ambitious, these might warrant their own example page at this point, so you could separate out the examples / snippets. It's getting busy.

Other than the glitch issue, I think this is mergeable and my comments are only suggestions.

border-width: 1px 0;
// Offset the background color from the border by 1px
padding: 1px;
background-clip: content-box;
Copy link
Contributor

Choose a reason for hiding this comment

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

wut do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added more to the comment above


export interface EuiButtonGroupIdToSelectedMap {
[id: string]: boolean;
}

export type GroupButtonSize = 's' | 'm';
export type GroupButtonSize = 's' | 'm' | 'compressed';
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you could get away with changing this to xs to match the actual buton props. It, similarly, mostly changes the font size.

I know we're running into this a lot, and I'll throw my hands up the same way, but that's just a thought.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I had considered this, but I think matching to the same name as the form compressed stuff will help connect the ideas that this should be used within compressed forms.

@cchaos
Copy link
Contributor Author

cchaos commented Sep 17, 2019

@snide I fixed the glitch

@ryankeairns I updated the screenshot in the summary to match the styles we collaborated on.

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

LGTM. Thanks for the updates

@cchaos cchaos merged commit e0ef3de into elastic:master Sep 18, 2019
@cchaos cchaos deleted the compressed-button-group branch September 18, 2019 12:33
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.

3 participants