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

Flyout alterations #925

Merged
merged 2 commits into from
Jun 14, 2018
Merged

Flyout alterations #925

merged 2 commits into from
Jun 14, 2018

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Jun 13, 2018

  • Added “cross” close button to top right as default if (“onClose”) is present
  • Added option for a border below header
  • Altered doc examples to represent how we want the flyouts to display consistently

screen shot 2018-06-13 at 15 09 31 pm

@snide I made some design decisions on the header, let me know if you think otherwise.

- Added “cross” close button to top right as default if (“onClose”) is present
- Added option for a border below header
- Altered doc examples to represent how we _want_ the flyouts to display consistently
Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

LGTM! pulled code and ran through the examples; tested escape-key functionality still works when hiding the close button.

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.

This looks great. Thanks for picking it up.

Don't forget the changelog before you merge. Because people were adding these on their own, and the default ability will add them (which makes sense) we should prolly add some sort of warning in the changelog more than usual. It's not a breaking change, but people should be aware to remove their additive stuff.

/**
* Locks the mouse / keyboard focus to within the flyout
*/
ownFocus: PropTypes.bool,
};

EuiFlyout.defaultProps = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you set defaults for the false values here just so the docs read a little better.

image

- Added default bool props
- changelog
@cchaos cchaos merged commit 9669e8b into elastic:master Jun 14, 2018
@cchaos cchaos deleted the flyout-close-clean-branch branch June 14, 2018 16:18
@snide
Copy link
Contributor

snide commented Jun 14, 2018

cc @ycombinator, @justinkambic. I noticed you were adding these close buttons manually in the Pipeline viewer. We're just going to make it a default EUI behavior. When this merges down into Kibana, you'll want to remove your additional buttons so that we don't have duplicate functionality.

@ycombinator
Copy link
Contributor

ycombinator commented Jun 18, 2018

@snide Thanks for the heads up. BTW, grepping through the Kibana code it looks like there are manually-added close buttons in quite a few other places too. I'm not sure how all those other places of EuiFlyout want to handle the change, especially given that some have their close buttons in the footer.

@snide
Copy link
Contributor

snide commented Jun 18, 2018

The close buttons in the footer should be fine. Generally they shouldn't need them anymore, but they won't visually "break" things.

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.

4 participants