Skip to content

Conversation

@posth
Copy link
Contributor

@posth posth commented Mar 31, 2020

WHY are these changes introduced?

This PR fixes an overflow and wrapping styling issue on the DescriptionList component.

More specifically, that long strings passed to the keys (term and description) of an Item in the items props of the component will not conserve their specific styling widths.

Sample codebox with the issue
Screen Shot 2020-03-31 at 8 58 37 AM

WHAT is this pull request doing?

We add a couple css styles to not have long content overflow. This lets the component maintain it's already enforced widths for long content passed in.

with this styling fix applied
Screen Shot 2020-03-31 at 8 42 51 AM
Copy-paste this code in playground/Playground.tsx:
import React from 'react';

import {Page, DescriptionList} from '../src';

export function Playground() {
  return (
    <Page title="Playground">
      <DescriptionList
        items={[
          {
            term:
              'termtermtermtermtermtermtermtermtermtermtermtermtermtermtermtermtermtermtermtermtermtermtermtermtermtermtermtermtermterm',
            description:
              'longdescriptionlongdescriptionlongdescriptionlongdescriptionlongdescriptionlongdescriptionlongdescriptionlongdescription',
          },
          {
            term:
              'termtermtermtermtermtermtermtermtermtermtermtermtermtermtermtermtermtermtermtermtermtermtermtermtermtermtermtermtermterm',
            description:
              'longdescriptionlongdescriptionlongdescriptionlongdescriptionlongdescriptionlongdescriptionlongdescriptionlongdescription',
          },
          {
            term:
              'termtermtermtermtermtermtermtermtermtermtermtermtermtermtermtermtermtermtermtermtermtermtermtermtermtermtermtermtermterm',
            description:
              'longdescriptionlongdescriptionlongdescriptionlongdescriptionlongdescriptionlongdescriptionlongdescriptionlongdescription',
          },
        ]}
      />
    </Page>
  );
}

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

🎩 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

@posth posth force-pushed the description_item_css_fix branch from b7388fb to 450d9d5 Compare March 31, 2020 13:00
@github-actions
Copy link
Contributor

github-actions bot commented Mar 31, 2020

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

Details:
All files potentially affected (total: 1)
📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

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

Files potentially affected (total: 1)

@posth posth requested review from chloerice and loic-d March 31, 2020 13:19
Copy link
Contributor

@loic-d loic-d left a comment

Choose a reason for hiding this comment

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

I would double check with UX if it makes sense to have an ellipsis overflow for the text cc/ @chloerice
🎩 looks great. If we want to move in this direction, I would apply a similar fix for mobile.

@posth posth force-pushed the description_item_css_fix branch 6 times, most recently from 2ffd60d to 4fad74f Compare April 6, 2020 13:03
@posth posth force-pushed the description_item_css_fix branch from 4fad74f to ad86c5f Compare April 14, 2020 17:38
@ghost ghost added the cla-needed Added by a bot. Contributor needs to sign the CLA Agreement. label Apr 14, 2020
Copy link
Member

@chloerice chloerice left a comment

Choose a reason for hiding this comment

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

Thanks for catching this, Marc! Adding word-break: break-word to .DescriptionList is all that's needed here to prevent overflow and enable long words to wrap like regular sentences do. If you need the content to be truncated instead of wrapping, you can wrap with Truncate.

Large screen Small screen
Screen Shot 2020-04-16 at 1 03 55 PM Screen Shot 2020-04-16 at 1 03 46 PM
Click to view collapsed playground code
import React from 'react';

import {Page, DescriptionList} from '../src';

export function Playground() {
  return (
    <Page title="Playground">
      <DescriptionList
        items={[
          {
            term: 'Logistics',
            description:
              'The management of products or other resources as they travel between a point of origin and a destination.',
          },
          {
            term:
              'The management of products or other resources as they travel between a point of origin and a destination.',
            description:
              'The management of products or other resources as they travel between a point of origin and a destination.',
          },
          {
            term: 'Sole proprietorship',
            description:
              'A business structure where a single individual both owns and runs the company.',
          },
          {
            term: 'Discount code',
            description:
              'A series of numbers and/or letters that an online shopper may enter at checkout to get a discount or special offer.',
          },
        ]}
      />
    </Page>
  );
}





@posth
Copy link
Contributor Author

posth commented Apr 17, 2020

Adding word-break: break-word to .DescriptionList is all that's needed here to prevent overflow and enable long words to wrap like regular sentences do.

Much more succinct 💯

@ghost ghost removed the cla-needed Added by a bot. Contributor needs to sign the CLA Agreement. label Apr 17, 2020
@posth posth force-pushed the description_item_css_fix branch 2 times, most recently from 6bc917f to 9e9cb62 Compare April 17, 2020 18:51
@chloerice chloerice dismissed their stale review April 17, 2020 20:39

Requested changes made ✅

Copy link
Member

@chloerice chloerice left a comment

Choose a reason for hiding this comment

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

🚢

@posth posth force-pushed the description_item_css_fix branch from 03ff8c8 to 01aeed3 Compare April 20, 2020 12:35
@posth posth force-pushed the description_item_css_fix branch from 5f52b6d to 32e7e83 Compare April 20, 2020 12:52
@posth posth merged commit 87598ab into master Apr 20, 2020
@posth posth deleted the description_item_css_fix branch April 20, 2020 13:55
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.

3 participants