[K7] Add KeyPadMenu, Popover, and Header components. #13087

cjcenizal · 2017-07-25T02:07:15Z

@snide I haven't Reactified the newer components you added, like the user popover or the link. Note that I made some changes to the reset and had to drop usage of our namespacing mixins in a couple places because they couldn't support my use case.

- Rename KeyPad to KeyPadMenu and PopMenu to Popover.

- Tweak reset to be less opinionated about link styles, and more opinionated about global font-family.

- HeaderLogo. - HeaderSection, HeaderSectionItem, HeaderSectionItemButton. - HeaderBreadcrumbs, HeaderBreadcrumb, HeaderBreadcrumbCollapsed.

cjcenizal · 2017-07-25T17:31:44Z

ui_framework/src/global_styling/reset/_reset.scss

@@ -25,6 +25,7 @@ time, mark, audio, video {
  border: 0;
  font-size: 100%;
  font: inherit;
+  font-family: $kuiFontFamily;


This overrides the default font-family Chrome applies to the button element via the font property.

snide

Did a quick pass.

snide · 2017-07-25T17:59:50Z

ui_framework/src/components/popover/popover.js

+  const classes = classNames(
+    'kuiPopover',
+    anchorPositionToClassNameMap[anchorPosition],
+    isOpen ? 'isOpen' : undefined,


For the popover component I would simply not render the popover body until it's clicked. There's no reason to keep it around in the dom when the page loads.

I'd remove the isOpen state and just apply the animation as a transition delay.

Once opened it should be dismissible by clicking outside the popover itself.

apply the animation as a transition delay.

What do you mean by this? Can you give an example?

snide · 2017-07-25T18:06:27Z

ui_framework/src/components/key_pad_menu/_key_pad_menu.scss

+  border-color: transparent;
+  border-radius: $kuiBorderRadius;
+
+  &:hover, &:focus {


Hover state and focus state are different. You converged them.

snide · 2017-07-25T18:18:04Z

ui_framework/src/components/popover/_popover.scss

@@ -1,7 +1,8 @@
 // Pop menu is an animated popover relatively positioned to a button / action.
 // By default it positions in the middle, but can be anchored left and right.

-@include component('kuiPopMenu') {
+@include component('kuiPopover') {


The changes with how you're doing positioning here break the usage.

The problem we need to solve is that the original way the Popover was implemented caused the arrow to overlap the content, which works great for the Header, but not so great for anything else. You can see this on the Popover example page:

Discussed on slack. Modifier for less common case (the header) to make it flush.

snide · 2017-07-25T18:20:09Z

ui_framework/src/components/popover/_popover.scss

@@ -19,18 +20,22 @@
    background: $kuiColorEmptyShade;
    position: absolute;
    min-width: $kuiSize * 16; // Can expand further, but this size is good for our menus.
-    top: 100%;
-    left: -$kuiSize * 8;
+    top: calc(100% + #{$kuiSizeS});


Avoid calcs if you can. They are pretty dirty and hard to read.

snide · 2017-07-25T18:23:32Z

ui_framework/src/global_styling/reset/_reset.scss

  }

  &:focus {
-    text-decoration: underline;
-    background: $kuiFocusBackgroundColor;
+    text-decoration: none;


These changes broke the focus state of a bunch of stuff (see breadcrumbs). These should be left in because they are a completely common pattern and I'd rather not have to write focus states and hover states for everything.

The reason I made this change was because KeyPadMenuItemButton wasn't picking up the underline on hover, since it's a <button> and not a link. This was a really subtle difference and I can see it being very easily overlooked by someone working on the UI Framework. Imagine we change the link behavior sometime in the future -- how will we know which components need to be updated to share this behavior?

What we really need is a single source of truth for link-like behavior, which can be consumed by all the components which need it, and which will be the one place we need to update when we want to change this behavior.

I think we should create a mixin to encapsulate this behavior and solve this problem. What do you think?

That's a valid point, but man that mixin is going to be everywhere.

I'm not too opinionated, but if you go the mixin route make sure to add it back to the breadcrumbs.

snide · 2017-07-25T18:29:36Z

ui_framework/src/components/header/_header_logo.scss

+  vertical-align: middle;
+
+  @include child('icon') {
+    height: $kuiSizeXL !important;


Don't use important.

We need it to override .kuiIcon--medium.

Why do we need kui--medium on the icon here? All it provides is size. This should be used instead of it, not on top of it?

It's the default for when no size is specified. To remove the !important, we have to remove the default size, which is fine with me.

snide · 2017-07-25T18:36:49Z

ui_framework/src/components/popover/_popover.scss

@@ -10,7 +11,7 @@
      opacity: 1;
      visibility: visible;
      z-index: 2;


This was a mistake of mine. You should use a variable for this... $kuiZContent

- Fix hover and focus states for KeyPadMenuItem. - Remove default size on Icon.

cjcenizal · 2017-07-25T20:55:40Z

@snide I addressed your feedback. I'm leaving the link mixin stuff for later, for when we create a Link React component and example page for it.

snide

Asked CJ to fix the focus states. Then we're all good.

- Create kuiLink mixin.

* Add Popover component. * Rename KeyPad to KeyPadMenu and PopMenu to Popover. * Add KeyPadMenu component. * Tweak reset to be less opinionated about link styles, and more opinionated about global font-family. - Create kuiLink mixin. * Add Header components. - HeaderLogo. - HeaderSection, HeaderSectionItem, HeaderSectionItemButton. - HeaderBreadcrumbs, HeaderBreadcrumb, HeaderBreadcrumbCollapsed. * Remove default size on Icon. * Fix linting errors.

cjcenizal added the Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins. label Jul 25, 2017

cjcenizal added 4 commits July 25, 2017 10:16

Add Popover component.

2f51907

- Rename KeyPad to KeyPadMenu and PopMenu to Popover.

Add KeyPadMenu component.

0f5d74a

- Tweak reset to be less opinionated about link styles, and more opinionated about global font-family.

Add Header components.

9f8022f

- HeaderLogo. - HeaderSection, HeaderSectionItem, HeaderSectionItemButton. - HeaderBreadcrumbs, HeaderBreadcrumb, HeaderBreadcrumbCollapsed.

Fix linting errors.

a05f120

cjcenizal force-pushed the k7/reactify-header-components branch from 22d0cdb to a05f120 Compare July 25, 2017 17:27

cjcenizal changed the title ~~[K7] Add KeyPadMenu and Popover components.~~ [K7] Add KeyPadMenu Popover, and Header components. Jul 25, 2017

cjcenizal requested a review from snide July 25, 2017 17:27

cjcenizal commented Jul 25, 2017

View reviewed changes

snide suggested changes Jul 25, 2017

View reviewed changes

snide reviewed Jul 25, 2017

View reviewed changes

cjcenizal added 2 commits July 25, 2017 12:48

Support clicking outside of a Popover to close it.

ecba80b

Add KeyPadMenu support for custom class on body.

0913e4e

- Fix hover and focus states for KeyPadMenuItem. - Remove default size on Icon.

snide approved these changes Jul 25, 2017

View reviewed changes

cjcenizal force-pushed the k7/reactify-header-components branch from 9497532 to 4e80d6f Compare July 25, 2017 21:29

Add user menu markup to Header example.

d6710ff

- Create kuiLink mixin.

cjcenizal force-pushed the k7/reactify-header-components branch from 4e80d6f to d6710ff Compare July 25, 2017 21:29

Adjust notification line-height.

995f4bc

cjcenizal changed the title ~~[K7] Add KeyPadMenu Popover, and Header components.~~ [K7] Add KeyPadMenu, Popover, and Header components. Jul 25, 2017

cjcenizal merged commit 9e6fcfd into elastic:k7-ui-framework Jul 25, 2017

cjcenizal deleted the k7/reactify-header-components branch July 25, 2017 21:33

cjcenizal mentioned this pull request Jul 25, 2017

K7 KUI #12807

Closed

51 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[K7] Add KeyPadMenu, Popover, and Header components. #13087

[K7] Add KeyPadMenu, Popover, and Header components. #13087

cjcenizal commented Jul 25, 2017 •

edited

Loading

cjcenizal Jul 25, 2017

snide left a comment

snide Jul 25, 2017

snide Jul 25, 2017

cjcenizal Jul 25, 2017 •

edited

Loading

snide Jul 25, 2017

snide Jul 25, 2017

cjcenizal Jul 25, 2017

snide Jul 25, 2017

snide Jul 25, 2017

snide Jul 25, 2017 •

edited

Loading

cjcenizal Jul 25, 2017 •

edited

Loading

snide Jul 25, 2017

snide Jul 25, 2017

cjcenizal Jul 25, 2017

snide Jul 25, 2017

cjcenizal Jul 25, 2017

snide Jul 25, 2017

cjcenizal commented Jul 25, 2017

snide left a comment

[K7] Add KeyPadMenu, Popover, and Header components. #13087

[K7] Add KeyPadMenu, Popover, and Header components. #13087

Conversation

cjcenizal commented Jul 25, 2017 • edited Loading

Choose a reason for hiding this comment

snide left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal Jul 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snide Jul 25, 2017 • edited Loading

Choose a reason for hiding this comment

cjcenizal Jul 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal commented Jul 25, 2017

snide left a comment

Choose a reason for hiding this comment

cjcenizal commented Jul 25, 2017 •

edited

Loading

cjcenizal Jul 25, 2017 •

edited

Loading

snide Jul 25, 2017 •

edited

Loading

cjcenizal Jul 25, 2017 •

edited

Loading