[EuiContextMenu] Add size prop #4409

andreadelrio · 2021-01-04T15:34:54Z

Summary

Added the prop size to EuiContextMenu to support a small size.

Added snippets to all the examples in this page
Slightly modified the text on the EuiContextMenuPanel section for more clarity.

Checklist

Check against all themes for compatibility in both light and dark modes
Checked in mobile
Checked in Chrome, Safari, Edge, and Firefox
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

kibanamachine · 2021-01-04T16:12:46Z

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

snide · 2021-01-04T16:14:08Z

Thanks, good pickup! This one always felt too airy to me. Should we possibly make this the default size (since it just affects padding)? cc @cchaos

cchaos · 2021-01-04T18:53:30Z

🎉 Thanks @andreadelrio for getting this one started! I think we can take it even further than just adjusting the padding.

They way I think about our components is how they're going to be used and common scenarios. For instance, with context menus, there are 3 main usages

Top (application) menu item, like Share and Export. These benefit from being the spacious medium size as each item will perform a "major" action.
Repeatable action menu like in tables showing options per item. These benefit from the more compact version because they affect only a single item and are repeated per row.
Toolbar actions like in the Data Grid's column sort. These benefit from an even more compact version because they most likely have a lot of options.

For now, we'll concentrate on the first 2 options. So to address the "default" comment, I think it's important to not assume intent and err on keeping the current display as default. We can adjust component level implementations, like the EuiBasicTableActions component to use the small size, but I don't think we should change the default for all usages.

As for the current design, I think the small version could also benefit from a decrease in font size. It's still using the base 16px font size which is quite large for usages like bullet 2. Also decreasing the font-size but keeping this new padding size will give the options a little more separation which I think they need.

Here's a re-paste of the original idea/screenshot:

Also based on the descriptions above, it would be great to include some "guidelines" about when it's best to use which size in the documentation.

andreadelrio · 2021-01-05T16:48:51Z

@cchaos thanks for the feedback. I adjusted the font size and added some guidelines to the docs.

kibanamachine · 2021-01-05T17:11:30Z

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

cchaos

Thanks for making those changes. The screenshots you pasted of the default theme look great, but I think there's still some work to do for the Amsterdam theme.

The spacing doesn't actually seem as tightened up as it could be, my suggestion would be to reduce just the top/bottom padding a bit like:

Also, the font size of the title is now too small. It should really stay at 14px aka $euiFontSizeS and then match the same padding as mentioned above:

CHANGELOG.md

src-docs/src/views/context_menu/context_menu_example.js

src-docs/src/views/context_menu/small.js

src/components/context_menu/context_menu.tsx

kibanamachine · 2021-01-06T18:58:28Z

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

kibanamachine · 2021-01-06T19:40:30Z

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

kibanamachine · 2021-01-09T01:36:55Z

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

kibanamachine · 2021-01-11T19:07:06Z

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

- Swapped actual examples between Sizes and Single panel - Added some more to a few snippets

- Also updated the euiPopoverTitle mixin to accept a size prop and re-created it for Amsterdam for better adjustments.

Context menu follow up

kibanamachine · 2021-01-13T17:41:26Z

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

kibanamachine · 2021-01-13T18:56:38Z

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

src/components/context_menu/__snapshots__/context_menu_panel.test.tsx.snap

thompsongl · 2021-01-14T16:48:35Z

PR4U: andreadelrio#14

Prevents passing undefined size through to menu items

kibanamachine · 2021-01-14T17:22:51Z

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

cchaos

👍 LGTM! But Jenkins is failing because of an accessibility issue:

12:19:19 [duplicate-id]: Ensures every id attribute value is unique
12:19:19   Help: https://dequeuniversity.com/rules/axe/3.5/duplicate-id?application=axe-puppeteer
12:19:19   Elements:
12:19:19     - #context-menu > div:nth-child(2) > .euiPopover--anchorDownLeft.euiPopover
12:29:19 1 accessibility errors

kibanamachine · 2021-01-14T19:02:01Z

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

andreadelrio · 2021-01-14T19:53:35Z

Jenkins, test this

cchaos

🎉

kibanamachine · 2021-01-14T20:14:49Z

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

thompsongl

Just the type changes, otherwise LGTM!

src/components/context_menu/context_menu.tsx

thompsongl · 2021-01-14T21:48:06Z

src/components/context_menu/context_menu.tsx

+    /**
+     * Alters the size of the items and the title
+     */
+    size?: typeof SIZES[number];


Suggested change

size?: typeof SIZES[number];

size?: keyof typeof sizeToClassNameMap;

thompsongl · 2021-01-14T21:48:28Z

src/components/context_menu/context_menu_item.tsx

+  /**
+   * Reduce the size to `s` when in need of a more compressed menu
+   */
+  size?: typeof SIZES[number];


Suggested change

size?: typeof SIZES[number];

size?: keyof typeof sizeToClassNameMap;

thompsongl · 2021-01-14T21:48:55Z

src/components/context_menu/context_menu_panel.tsx

+  /**
+   * Alters the size of the items and the title
+   */
+  size?: typeof SIZES[number];


Suggested change

size?: typeof SIZES[number];

size?: keyof typeof titleSizeToClassNameMap;

Co-authored-by: Greg Thompson <[email protected]>

kibanamachine · 2021-01-14T22:42:32Z

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

andreadelrio added 6 commits December 4, 2020 13:22

add small size

c63002d

add keyboard

edc33a1

add small size

0fd70c4

fix error

0ee90df

Merge remote-tracking branch 'upstream/master' into contextMenuM

129ae3c

cl

5ae0dd3

adjust font size and docs

794e9c9

andreadelrio requested a review from cchaos January 5, 2021 17:08

cchaos suggested changes Jan 5, 2021

View reviewed changes

feedback, wip

a4db58c

andreadelrio added 2 commits January 6, 2021 14:09

wording and cleanup

dcfe689

revert main example

10ca7c6

andreadelrio requested a review from cchaos January 6, 2021 21:59

cleanup

6a9878b

andreadelrio force-pushed the contextMenuM branch from 58b152e to 6a9878b Compare January 9, 2021 01:17

andreadelrio added 2 commits January 11, 2021 10:37

re-simplifying small example

6e4e582

remove one line

0177632

cchaos and others added 4 commits January 11, 2021 15:26

Fixing up examples

d8aeabd

- Swapped actual examples between Sizes and Single panel - Added some more to a few snippets

Creating a specific small title class instead of relying on nesting

502cc48

- Also updated the euiPopoverTitle mixin to accept a size prop and re-created it for Amsterdam for better adjustments.

Adding prop comments and a few more tests

7ab6a83

Merge pull request #13 from cchaos/contextMenuM

60bf2cf

Context menu follow up

cl

e2e59ea

andreadelrio requested a review from thompsongl January 13, 2021 20:22

cchaos reviewed Jan 13, 2021

View reviewed changes

src/components/context_menu/__snapshots__/context_menu_panel.test.tsx.snap Outdated Show resolved Hide resolved

prevent undefined size

c076074

Merge pull request #14 from thompsongl/contextMenuM

d73573a

Prevents passing undefined size through to menu items

cchaos reviewed Jan 14, 2021

View reviewed changes

fix a11y issue

d8a0455

cchaos approved these changes Jan 14, 2021

View reviewed changes

thompsongl approved these changes Jan 14, 2021

View reviewed changes

andreadelrio and others added 2 commits January 14, 2021 17:09

Update src/components/context_menu/context_menu.tsx

12259af

Co-authored-by: Greg Thompson <[email protected]>

changes to types

a380116

andreadelrio merged commit d16d296 into elastic:master Jan 14, 2021

chandlerprall mentioned this pull request Jan 20, 2021

Upgrade EUI to v31.3.0 elastic/kibana#88881

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiContextMenu] Add size prop #4409

[EuiContextMenu] Add size prop #4409

andreadelrio commented Jan 4, 2021 •

edited

Loading

kibanamachine commented Jan 4, 2021

snide commented Jan 4, 2021

cchaos commented Jan 4, 2021

andreadelrio commented Jan 5, 2021

kibanamachine commented Jan 5, 2021

cchaos left a comment

kibanamachine commented Jan 6, 2021

kibanamachine commented Jan 6, 2021

kibanamachine commented Jan 9, 2021

kibanamachine commented Jan 11, 2021

kibanamachine commented Jan 13, 2021

kibanamachine commented Jan 13, 2021

thompsongl commented Jan 14, 2021

kibanamachine commented Jan 14, 2021

cchaos left a comment

kibanamachine commented Jan 14, 2021

andreadelrio commented Jan 14, 2021

cchaos left a comment

kibanamachine commented Jan 14, 2021

thompsongl left a comment

thompsongl Jan 14, 2021

thompsongl Jan 14, 2021

thompsongl Jan 14, 2021

kibanamachine commented Jan 14, 2021

	size?: typeof SIZES[number];
	size?: keyof typeof sizeToClassNameMap;

	size?: typeof SIZES[number];
	size?: keyof typeof titleSizeToClassNameMap;

[EuiContextMenu] Add size prop #4409

[EuiContextMenu] Add size prop #4409

Conversation

andreadelrio commented Jan 4, 2021 • edited Loading

Summary

Checklist

kibanamachine commented Jan 4, 2021

snide commented Jan 4, 2021

cchaos commented Jan 4, 2021

andreadelrio commented Jan 5, 2021

kibanamachine commented Jan 5, 2021

cchaos left a comment

Choose a reason for hiding this comment

kibanamachine commented Jan 6, 2021

kibanamachine commented Jan 6, 2021

kibanamachine commented Jan 9, 2021

kibanamachine commented Jan 11, 2021

kibanamachine commented Jan 13, 2021

kibanamachine commented Jan 13, 2021

thompsongl commented Jan 14, 2021

kibanamachine commented Jan 14, 2021

cchaos left a comment

Choose a reason for hiding this comment

kibanamachine commented Jan 14, 2021

andreadelrio commented Jan 14, 2021

cchaos left a comment

Choose a reason for hiding this comment

kibanamachine commented Jan 14, 2021

thompsongl left a comment

Choose a reason for hiding this comment

thompsongl Jan 14, 2021

Choose a reason for hiding this comment

thompsongl Jan 14, 2021

Choose a reason for hiding this comment

thompsongl Jan 14, 2021

Choose a reason for hiding this comment

kibanamachine commented Jan 14, 2021

andreadelrio commented Jan 4, 2021 •

edited

Loading