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

Some random fixes #546

Merged
merged 14 commits into from
Mar 22, 2018
Merged

Some random fixes #546

merged 14 commits into from
Mar 22, 2018

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Mar 20, 2018

Modals

  1. Adjusting modals for mobile

    • Added drop shadows for overflows
    • Added padding to header to accomodate close button
    • Modal buttons should be normal size (not small) (docs only)

screen shot 2018-03-20 at 15 18 14 pm

  1. Fixes Dark theme modals have low-contrast against the background #334 - Lightening overlay color in dark theme

screen shot 2018-03-20 at 15 17 22 pm

  1. Fixes EuiConfirmModal--option to set confirm button to red #520 Added a type prop to EuiConfirmModal

    • Defaults to ‘confirm’ (positive/neutral), but can be ‘destroy’.

screen shot 2018-03-20 at 15 26 42 pm

Non-issues

  1. Overflowing tabs now scroll horizontally

Issues

  1. Fixes [EuiButtonEmpty] The flush property makes focus background uneven #537 - Removing padding from both sides of EuiEmptyButton when flush

screen shot 2018-03-20 at 15 25 45 pm

  1. Fixes Fix accessibility issues in pagination #474 - Added disabled prop to placeholder (ellipses) pagination

  2. Fixes <EuiFlexGroup> alignItems='baseline' support #423 - Added ‘baseline’ as option to EuiFlexGroup’s alignItems prop

    • ’normal’ doesn’t do anything
  3. Fixes euiHeader__notification needs to be a component? #87 - Converted .euiHeader__notification into EuiHeaderNotification

@@ -53,6 +60,7 @@ export class EuiConfirmModal extends Component {
cancelButtonText,
confirmButtonText,
className,
type,
Copy link
Contributor

Choose a reason for hiding this comment

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

Think we should just make this prop buttonColor? Then you don't need the mapping and it's a little more flexible? Guessing we'll have other "types" beyond just these two, which are a little specifically named.

Copy link
Contributor Author

@cchaos cchaos Mar 21, 2018

Choose a reason for hiding this comment

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

That's a very good question and I think it comes down to how we want to be building these components for use cases. Semantically or stylistically? And how rigorously are we trying to ensure proper usage?

We could decide on stylistically, and use buttonColor (though which button do we mean), and then they (the consumers) have to decide on colors. Where as if we ask them what type we choose the colors for them. So they're more deciding what it's supposed to represent versus how should it look. I tend to lean towards the former and use more semantic options that are clear.

On the flip side, or rather, almost a compromise to this, is that our color naming is pretty semantic already. So perhaps instead of asking them what buttonColor, we still ask them what type but that type must be .oneOf(BUTTON_COLORS).

So that, for instance type="danger" or type="success" would color the button appropriately but also allow us to do any extra styling we may want (like color the title or add a giant flashing stop sign).

@cchaos cchaos merged commit f47e4ec into elastic:master Mar 22, 2018
@cchaos cchaos deleted the fixes branch March 22, 2018 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment