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

Updates to EuiSelectableTemplateSitewide and some other fixes for the new header #4008

Merged
merged 20 commits into from
Sep 14, 2020

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Sep 4, 2020

Fixes #3988

Updates the text color of the search input when in the dark header to be more visible. The placeholder still shouldn't be too prominent, so it's only slightly lighter.

Screen Shot 2020-09-04 at 17 46 21 PM

Fixes #3946

EuiHeaderSectionItemButton now accepts a boolean for notification and when set tp true will simply display a dot icon.

Screen Shot 2020-09-04 at 12 11 51 PM

Handling of a "mobile" version of EuiSelectableTemplateSitewide

New props were added to the component for providing the ability to trigger the whole component with a separate button. This prop is called popoverButton. Which, when provided, will display this inline while putting the search component inside of the popover. It also adds popoverButtonMaxBreakpoint to allow setting a certain point as to when to stop using the popoverButton.

If neither are provided, it simply continues to display as before. Here is an example of it working in the header.

Screen Recording 2020-09-04 at 12 20 19 PM

EuiHeaderLink colors

Prior to this PR, they would always render as text color for inactive and blue for active, but now consumers can pass any color that is accepted by EuiLink. This supports Kibana's use of moving the menu link to the header. The main reason for this is so that they can get the mobile version out-of-the-box.

Screen Shot 2020-09-04 at 12 28 39 PM

EuiSelectable fix

The EuiSelectableList accidentally had it's showIcon prop removed during #3983. This adds that back in.

⚠️ Breaking changes

In the header example above, it was necessary to use the EuiHideFor and EuiShowFor components to move the display from the center items to the right side items in smaller windows. The original problem was that these components were simply hiding the elements with display: none but still causing them to render which duplicate a lot of code and can cause id & ref clashes.

Now that we have access to our breakpoints in JS, I've converted these components from using classes to simply rendering or not rendering their children based on the window width. This does now require children and may break instances of people using custom SASS breakpoints that override EUI's defaults. This shouldn't be the case in any of Elastic products, so I feel mostly safe there.

Also removes the display prop of EuiShowFor as we're no longer rendering a wrapping element.

--

I'm going to say that this PR closes out #3490 and #3489 in preference of opening up more specific issues that arise. 🎉
But all the bits should be in there and available by EUI.

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
Copy link

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

@kibanamachine
Copy link

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

1 similar comment
@kibanamachine
Copy link

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

@cchaos cchaos marked this pull request as ready for review September 4, 2020 21:45
@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@cchaos
Copy link
Contributor Author

cchaos commented Sep 10, 2020

@chandlerprall Sorry for the last minute change, but I think allowing the consumer to provide an array of breakpoints is better than just setting a cap because then they could be adjust for larger screens as well.

@kibanamachine
Copy link

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

@cchaos
Copy link
Contributor Author

cchaos commented Sep 10, 2020

Jenkins, test this

@kibanamachine
Copy link

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

Copy link
Contributor

@myasonik myasonik left a comment

Choose a reason for hiding this comment

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

Just some nits but nothing seems big or serious

(Just did a code review at this point, haven't rerun this since helping in a commit earlier)

src/components/responsive/hide_for.tsx Show resolved Hide resolved
src/components/responsive/hide_for.tsx Outdated Show resolved Hide resolved
src/components/responsive/show_for.tsx Show resolved Hide resolved
@kibanamachine
Copy link

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

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.

Apart from pending comments/conversations @myasonik made, this LGTM!

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants