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

[Amsterdam] New tab styles #4075

Closed
wants to merge 2 commits into from
Closed

Conversation

daveyholler
Copy link
Contributor

Summary

Introduces new tab styles for the Amsterdam theme.

Screen Shot 2020-09-24 at 9 01 21 AM

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Checked in Kibana within existing apps
    - [ ] Props have proper autodocs
    - [ ] Added documentation
    - [ ] Checked Code Sandbox works for the any docs 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

Special Attention

There will be some instances that require special attention when migrating in Kibana.

  1. Solutions (like Observability) that use tabs as navigation will need to address some CSS overrides.

Screen Shot 2020-09-23 at 1 54 44 PM

  1. [Discuss] Use cases where tab content with a shaded background is adjacent to the tab bar will need a spacer or margin.
    image

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4075/

@cchaos
Copy link
Contributor

cchaos commented Sep 24, 2020

Thanks as always @daveyholler for getting this one started. It makes it a ton easier to see the implications of the update.

My two concerns are that it can (1) easily degrade appearance as you pointed out it some usage examples. And (2) it now emphasizes all versions of the tabs which may not be the intention.

1. Most consumers have placed the tabs with the assumption it contains a border and no rounded corners

Places like in a flyout header or sidebar where they act as a divider will no longer work. Or where content buts up against to feel like it's a localized container to the content of the tabs (you showed an example from the EUI docs).

Screen Shot 2020-09-24 at 13 11 19 PM

2. The new emphasis on all versions may have adverse affects

Take for example when there are multiple tabs in a page layout (like this new one for Fleet), The only difference between the tab versions is boldness and/or font-size creating a bit of conflict in hierarchy.

Screen Shot 2020-09-24 at 13 28 48 PM


From my understanding of where this style was originally developed, was in context of using it as a more emphasized version of the "condensed" style for in-app navigation. This makes 100% sense to me. Therefore I propose, we update to this style only in the condensed version (which I think we can name better). So then we have a bit better of a hierachy like:

Screen Shot 2020-09-24 at 13 38 45 PM

I propose that ^^ ... Haha sorry

@johnbarrierwilson
Copy link
Member

At a low-level, I think the gray box looks great within the new Amsterdam styles. But, @cchaos raises some really good points.

Opinion: across the world wide web, tabs have always played the role of minimal-style navigational elements. Adding a large box around them changes the nature of tabs themselves.

At a high-level, I agree that it works okay in the navigation, but with the new solution sidenav possibly coming up, I'd suggest pressing pause on this until we come to a decision on the solution sidenav. Ultimately, if solution sidenav project comes up with an element that would replace all navigation like this, then it would change the outcome of this PR's update.

@cchaos
Copy link
Contributor

cchaos commented Sep 30, 2020

I'm going to throw this one back into draft mode until we have a path forward.

@cchaos cchaos marked this pull request as draft September 30, 2020 20:26
@cchaos cchaos removed their request for review September 30, 2020 20:26
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