Skip to content

[ResourceList] Trunctate long sortOption label #2957

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.

Sign up for GitHub

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

Merged
carolopolo merged 1 commit into from
May 4, 2020
Merged

[ResourceList] Trunctate long sortOption label #2957

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions UNRELEASED.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@

### Enhancements

- Truncated long sort options in `ResourceList` ([#2957](https://github.com/Shopify/polaris-react/pull/2957)

### Bug fixes

### Documentation
Expand Down
10 changes: 5 additions & 5 deletions src/components/ResourceList/ResourceList.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,11 +118,11 @@ $item-wrapper-loading-height: rem(64px);
}
}

// Force long sort options to be truncated
.SortWrapper,
.SortWrapper > * {
@include breakpoint-before(resource-list(breakpoint-small)) {
overflow: hidden;
.SortWrapper {
Copy link
Contributor Author

@carolopolo carolopolo May 4, 2020

Choose a reason for hiding this comment

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

@beefchimi this is a possible solution... tested on chrome, firefox & safari

but would love some input 🥬

Copy link
Contributor

@beefchimi beefchimi May 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Okay, so this can actually be fixed rather easily 😄

You can replace ALL of your changed code in this file with:

.SortWrapper {
  @include layout-flex-fix;

  // Required to apply styles to the <Labelled /> component
  > * {
    max-width: 100%;
  }
}

That is the most simple solution. Two things to potentially alter:

1. Apply "layout fix" to all children
Personally, I would apply that layout-flex-fix mixin to ALL children of .HeaderContentWrapper. So, you could instead do:

.HeaderTitleWrapper,
.CheckableButtonWrapper,
.AlternateToolWrapper,
.SortWrapper,
.SelectButtonWrapper {
  @include layout-flex-fix;
}

.SortWrapper {
  // Required to apply styles to the <Labelled /> component
  > * {
    max-width: 100%;
  }
}

2. Try to avoid reaching into other components to override styles
The > * selector is specifically targeting the .Labelled direct child. It would be preferable if we did not have to apply this snowflake override style... so I'm wondering if max-width: 100% can live comfortably on the .Labelled selector within Labelled.scss. I think its worth experimenting with that change to see if it causes issues any where else <Labelled /> is used.

Explanation
We need to utilize this layout-flex-fix mixin on direct children ("flex items") of a display: flex parent in order to get truncation to work correctly. This is a very common problem/solution! All that mixin is doing is applying the following styles:

min-width: 0;
max-width: 100%;

This seems... silly, because those prop + values seem obvious and should be default behaviour.... right?!?! The short answer is: flex box items will not shrink below their minimum content size. So, if the minimum content size is computed as the full width of a sentence, it cannot be below that width in order to truncate. In comes out handy "flex fix" styling to say "well actually min-width can be 0".

For a better explanation, our very own @dfmcphee wrote a blog post on it!
https://dfmcphee.com/flex-items-and-min-width-0/

Copy link
Contributor Author

@carolopolo carolopolo May 4, 2020

Choose a reason for hiding this comment

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

thanks so much @beefchimi! really appreciate that.

fyi for posterity's sake:

adding the max-width: 100% on the .Labelled selector in Labelled.scss results in:
image

adding the @include layout-flex-fix; to all children of .HeaderContentWrapper results in:
image

so i'm going with your very fist suggestion. thanks again for your input!

Copy link
Contributor

@beefchimi beefchimi May 4, 2020

Choose a reason for hiding this comment

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

We could improve this further with some tweaks to the other HeaderContentWrapper, mainly to that SelectWrapper so that it has a min-width of at least the:

  • width of a checkbox +
  • spacing()

But just so other reviews know - the label of that "select checkbox" will truncate as well. Its just that Caroline's example is soooo long that it goes beyond the text label and starts encroaching onto the checkbox input. Realistically, we probably wouldn't end up in the scenario.

@carolopolo would you mind opening an issue with your 2nd screenshot to track this? You can link to this comment + include any relevant context.

@include layout-flex-fix;

> * {
max-width: 100%;
}
}

Expand Down