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

Add Condensed Display Prop for EuiTabs & EuiTabbedContent #1904

Merged
merged 20 commits into from
May 2, 2019

Conversation

MichaelMarcialis
Copy link
Contributor

@MichaelMarcialis MichaelMarcialis commented Apr 30, 2019

Summary

This PR adds a new display prop to the EuiTabs and EuiTabbedContent components for the ability to use an alternative condensed style.

The hope is to use the new prop to display a slightly more compact, bolder and borderless tabbed navigation for use as top-level navigation in the SIEM app. This alternative style gives the navigation more prominence than it currently has and also differentiates itself from other usages of tabs further down the page.

image

Checklist

  • This was checked in mobile
  • This was checked in IE11
  • This was checked in dark mode
  • Any props added have proper autodocs
  • Documentation examples were added
  • A changelog entry exists and is marked appropriately
  • This was checked for breaking changes and labeled appropriately
  • Jest tests were updated or added to match the most common scenarios
  • This was checked against keyboard-only and screenreader scenarios
  • [ ] This required updates to Framer X components

@cchaos
Copy link
Contributor

cchaos commented May 1, 2019

Congrats on your first PR! 🍾

@snide mentioned that you had a particular use case for this style of tabs. Could you elaborate on that?

@MichaelMarcialis MichaelMarcialis marked this pull request as ready for review May 1, 2019 17:59
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Good job! The new prop was properly added, docs look good, SASS looks good, tests were added. Still need to:

Screen Shot 2019-05-01 at 17 11 17 PM


Styling

Do we think the underline is enough to indicate focus state?

In the default tabs we add a slight background color, same with empty buttons and links:

@chandlerprall
Copy link
Contributor

@cchaos would it be better to call the prop compressed to match the form element pattern?

@cchaos
Copy link
Contributor

cchaos commented May 2, 2019

@chandlerprall compressed isn't really the same thing here. The idea of condensed just means no full bottom border and less padding on the left and right, but it's still the original height and font size. Forms reduce their height but keep their left/right padding. So they're really not the same prop.

@snide
Copy link
Contributor

snide commented May 2, 2019

Yeah, I also gave him the recommendation to use a display prop instead of the boolean. In retrospect we've shot ourselves in the foot for stylistic props as booleans that really should just be "alternate displays". Gives us flexibility for the next variation without conflict.

@MichaelMarcialis MichaelMarcialis requested a review from cchaos May 2, 2019 18:26
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

LGTM, just had a request for the snippets and then I'd like either @thompsongl or @chandlerprall to give it a once-over.

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.

Code and docs changes LGTM

@cchaos
Copy link
Contributor

cchaos commented May 2, 2019

Looks like you're good to merge!

@MichaelMarcialis MichaelMarcialis merged commit 55330de into elastic:master May 2, 2019
@MichaelMarcialis MichaelMarcialis deleted the feature/tabs-update branch May 2, 2019 22:58
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