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

Allow EuiListGroupItem link text to wrap #1459

Merged
merged 7 commits into from
Jan 23, 2019

Conversation

ryankeairns
Copy link
Contributor

Summary

In testing the new side nav in Kibana, it is preferred that long link text in the flyout be allowed to wrap. The flyout has more vertical space than the main menu and it is common for saved objects to have lengthy titles.

This PR adds an isWrappable prop to EuiListGroupItem that provide the ability to make the text wrap as desired. The docs examples for the Nav Drawer shows the result. pictured below.

screenshot 2019-01-18 15 56 13

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

Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

One comment, rest looks good.

src/components/list_group/list_group_item.js Outdated Show resolved Hide resolved
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.

I just realized that it might be really cumbersome to have to apply this to each individual item. Can you also allow it to be passed via the full EuiListGroup?

@ryankeairns
Copy link
Contributor Author

@cchaos Going a step further, perhaps it should only be a prop on the EuiListGroup and not the EuiListGroupItem. I can't think of situation or very compelling reason to only wrap certain items in a list. Thoughts on that?

@cchaos
Copy link
Contributor

cchaos commented Jan 22, 2019

Well a consumer may not always be passing in items via the listItems but may just want to build it from scratch in which case they'd need to be able to pass the prop on the EuiListGroupItem itself.

@ryankeairns
Copy link
Contributor Author

I think even in that case you'd still have an <EuiListGroup displayType='wrapText' ...> wrapper around your <EuiListGroupItem>s, right? I'm slowly thinking this through, step by step :)

@cchaos
Copy link
Contributor

cchaos commented Jan 22, 2019

But then you'd have to do something like React.cloneChildren and re-apply the prop to each EuiListGroupItem. Unless you're thinking that you'd move the SASS from

.euiListGroupItem--wrapText {
  .euiListGroupItem__button { ... }
}

to

.euiListGroup--wrapText {
  .euiListGroupItem__button { ... }
}

?

Though, I think just allowing it be provided to both is the most flexible.

@ryankeairns
Copy link
Contributor Author

@snide The DISPLAY_TYPES are now being exported and used in the PropTypes.

@cchaos This latest commit moves the prop to the EuiListGroup. I'm leaning towards having the prop on the parent EuiListGroup only since it simplifies the interface for an EUI user on how this works (i.e. you won't see the prop on both the parent and children) and to avoid situations where some items would be wrapped an others are not. Perhaps we can start with this more restrictive approach and add the item prop later if users request it. Sound ok?

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.

Sure that's fine. I had a couple nit-picky things but I'll defer to your judgement. LG!

src/components/list_group/list_group.js Outdated Show resolved Hide resolved
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.

Seems to be working well. It's kind of odd to see the scrollbars and extra actions disappear before the sidebar starts animating closed, but those are things we can tinker with later.

Just a comment about min-width below.

src/components/list_group/_list_group_item.scss Outdated Show resolved Hide resolved
src/components/list_group/_list_group_item.scss Outdated Show resolved Hide resolved
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.

JS changes LGTM

@cchaos
Copy link
Contributor

cchaos commented Jan 23, 2019

Oh and you'll want to check IE11 again. The icons for favorites and recents are getting cut off in collapsed mode. Also, it's not respecting the scroll back up after re-opening:

@ryankeairns ryankeairns merged commit 8f36ae1 into elastic:master Jan 23, 2019
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