[ResourceList] Trunctate long sortOption label #2957

carolopolo · 2020-04-30T19:44:39Z

WHY are these changes introduced?

Fixes https://github.com/Shopify/store/issues/14834

WHAT is this pull request doing?

No matter the viewport size, it truncates the sortOption label if it doesn't fit. Previously, we were only truncating for mobile/small screens. This PR addresses running into the same problem on various screens.

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:

import React, {useState} from 'react';

import {Avatar, Card, ResourceItem, ResourceList, TextStyle} from '../src';

export function Playground() {
  const [sortValue, setSortValue] = useState('DATE_MODIFIED_DESC');

  const resourceName = {
    singular: 'customer',
    plural: 'customers',
  };

  const items = [
    {
      id: 341,
      url: 'customers/341',
      name: 'Mae Jemison',
      location: 'Decatur, USA',
    },
    {
      id: 256,
      url: 'customers/256',
      name: 'Ellen Ochoa',
      location: 'Los Angeles, USA',
    },
  ];

  return (
    <Card>
      <ResourceList
        selectable
        selectedItems={[]}
        resourceName={resourceName}
        items={items}
        renderItem={renderItem}
        sortValue={sortValue}
        sortOptions={[
          {
            label:
              'Last shrimpy shrimp created (ascending shrimp) shrimp shrimp shrimp shrimp shrimp',
            value: 'DATE_MODIFIED_DESC',
          },
          {label: 'Oldest update', value: 'DATE_MODIFIED_ASC'},
        ]}
        onSortChange={(selected) => {
          setSortValue(selected);
          console.log(`Sort option changed to ${selected}.`);
        }}
      />
    </Card>
  );

  function renderItem(item) {
    const {id, url, name, location} = item;
    const media = <Avatar customer size="medium" name={name} />;

    return (
      <ResourceItem
        id={id}
        url={url}
        media={media}
        accessibilityLabel={`View details for ${name}`}
      >
        <h3>
          <TextStyle variation="strong">{name}</TextStyle>
        </h3>
        <div>{location}</div>
      </ResourceItem>
    );
  }
}

🎩 checklist

Tested on mobile
Tested on multiple browsers
Tested for accessibility
Updated the component's README.md with documentation changes
Tophatted documentation changes in the style guide
For visual design changes, pinged one of @ HYPD, @ mirualves, @ sarahill, or @ ry5n to update the Polaris UI kit

github-actions · 2020-04-30T19:46:52Z

🟢 This pull request modifies 2 files and might impact 1 other files.

Details:

All files potentially affected (total: 1)

/src/components/ResourceList/ResourceList.tsx

📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

🎨 src/components/ResourceList/ResourceList.scss (total: 1)

Files potentially affected (total: 1)

/src/components/ResourceList/ResourceList.tsx

loic-d

🎩 LGTM

I think it makes sense to have the ellipsis on all viewports and not just small devices.
Would be great to get a second 👀 from a Polaris expert.

dleroux · 2020-05-04T13:57:35Z

Looks like the focus ring gets cut off with the overflow hidden:

carolopolo · 2020-05-04T16:06:54Z

src/components/ResourceList/ResourceList.scss

+//   overflow: hidden;
+// }
+
+.SortWrapper {


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

but would love some input 🥬

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/

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:

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

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

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.

beefchimi

Alternate solution suggested in my review 😄

beefchimi · 2020-05-04T17:14:25Z

src/components/ResourceList/ResourceList.scss

+//   overflow: hidden;
+// }
+
+.SortWrapper {


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/

beefchimi

Looks great!

beefchimi · 2020-05-04T18:30:44Z

src/components/ResourceList/ResourceList.scss

+//   overflow: hidden;
+// }
+
+.SortWrapper {


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.

ghost · 2020-05-04T19:05:11Z

🎉 Thanks for your contribution to Polaris React!

carolopolo marked this pull request as draft April 30, 2020 19:44

carolopolo changed the title ~~refactor breakpoint to include more screens~~ [ResourceList] Trunctate long sortOption label Apr 30, 2020

carolopolo force-pushed the resource-list/fix-sort-overflow branch from 7e3a7a5 to c875edf Compare April 30, 2020 19:57

carolopolo marked this pull request as ready for review April 30, 2020 19:58

carolopolo requested a review from loic-d April 30, 2020 19:58

carolopolo self-assigned this Apr 30, 2020

carolopolo requested review from chloerice and dleroux April 30, 2020 19:58

loic-d approved these changes Apr 30, 2020

View reviewed changes

carolopolo marked this pull request as draft May 4, 2020 16:04

carolopolo commented May 4, 2020

View reviewed changes

carolopolo removed request for chloerice and dleroux May 4, 2020 16:31

beefchimi reviewed May 4, 2020

View reviewed changes

fix long sort option label overflow on all screens

d781690

carolopolo force-pushed the resource-list/fix-sort-overflow branch from 65a19e4 to d781690 Compare May 4, 2020 17:59

carolopolo marked this pull request as ready for review May 4, 2020 17:59

beefchimi approved these changes May 4, 2020

View reviewed changes

carolopolo merged commit f5f542d into master May 4, 2020

carolopolo deleted the resource-list/fix-sort-overflow branch May 4, 2020 19:05

carolopolo mentioned this pull request May 4, 2020

[ResourceList] Improve styling of .HeaderContentWrapper children #2961

Closed

amorriscode mentioned this pull request May 28, 2020

[ResourceList] Adjust sizing of CheckableButtonWrapper so it doesn't get squished #3010

Closed

6 tasks

[ResourceList] Trunctate long sortOption label #2957

[ResourceList] Trunctate long sortOption label #2957

Uh oh!

Conversation

carolopolo commented Apr 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

WHY are these changes introduced?

WHAT is this pull request doing?

How to 🎩

🎩 checklist

Uh oh!

github-actions bot commented Apr 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Files potentially affected (total: 0)

Files potentially affected (total: 1)

Uh oh!

loic-d left a comment

Choose a reason for hiding this comment

Uh oh!

dleroux commented May 4, 2020

Uh oh!

carolopolo May 4, 2020

Choose a reason for hiding this comment

Uh oh!

beefchimi May 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carolopolo May 4, 2020

Choose a reason for hiding this comment

Uh oh!

beefchimi May 4, 2020

Choose a reason for hiding this comment

Uh oh!

beefchimi left a comment

Choose a reason for hiding this comment

Uh oh!

beefchimi May 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beefchimi left a comment

Choose a reason for hiding this comment

Uh oh!

beefchimi May 4, 2020

Choose a reason for hiding this comment

Uh oh!

ghost commented May 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

carolopolo commented Apr 30, 2020 •

edited

Loading

github-actions bot commented Apr 30, 2020 •

edited

Loading

beefchimi May 4, 2020 •

edited

Loading

beefchimi May 4, 2020 •

edited

Loading