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

fix: tag not using elipses when width constrained #4518

Merged
merged 6 commits into from
Dec 5, 2019

Conversation

lee-chase
Copy link
Member

Closes #4511

Makes the tag/filter to show the text elipses when the tag is too large for the

Changelog

Changed

M packages/components/src/components/tag/_tag.scss
M packages/components/src/components/tag/tag.hbs

Testing / Reviewing

Ran up base components storybook for both filter and tag. Used developer tools to constrain the width of the containing div.

@lee-chase lee-chase requested a review from a team as a code owner November 1, 2019 11:41
@ghost ghost requested review from dakahn and jnm2377 November 1, 2019 11:41
@emyarod emyarod requested a review from a team November 1, 2019 13:28
@ghost ghost requested review from designertyler and removed request for a team November 1, 2019 13:28
Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

looks good to me pending design approval

filter

image

standard

image

cursor: pointer;
padding-right: rem(2px); // Align with hover circle of X button
padding-right: rem(26px); // Space for hover circle and X button
Copy link
Member

@emyarod emyarod Nov 1, 2019

Choose a reason for hiding this comment

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

just to confirm the 26px is from 20px width svg, 2px left border, 2px right border, and 2px space from the right edge right? should we express it as this?

Suggested change
padding-right: rem(26px); // Space for hover circle and X button
padding-right: calc(#{$carbon--spacing-06} + #{rem(2px)}); // icon width + 2px space from right edge

Copy link
Contributor

Choose a reason for hiding this comment

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

bump @lee-chase if you have a sec!

@netlify
Copy link

netlify bot commented Nov 1, 2019

Deploy preview for carbon-components-react ready!

Built with commit 8e98b8e

https://deploy-preview-4518--carbon-components-react.netlify.com

@netlify
Copy link

netlify bot commented Nov 1, 2019

Deploy preview for the-carbon-components ready!

Built with commit 8e98b8e

https://deploy-preview-4518--the-carbon-components.netlify.com

@netlify
Copy link

netlify bot commented Nov 1, 2019

Deploy preview for carbon-elements ready!

Built with commit 8e98b8e

https://deploy-preview-4518--carbon-elements.netlify.com

Copy link
Contributor

@jendowns jendowns left a comment

Choose a reason for hiding this comment

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

a11y question --

If/when text is truncated, how would someone be able to see/access the full text of the tag?

@joshblack
Copy link
Contributor

joshblack commented Nov 6, 2019

@jendowns if I understand 2.1 requirements correctly, truncated text will need a tooltip. For 2.0, I believe it needs the title prop.

One question I had for this, do we want to go down the route of truncated text in the tag or do we just want it to scale as the text overflows? Seems like making sure it scales would help in our text resize situations but let me know if I'm missing something here!

@laurenmrice
Copy link
Member

laurenmrice commented Nov 14, 2019

To joshs comment my gut feeling is I don't think we should have truncated text inside a tag. I think what is in the tag should be visible at all times. But maybe there are some use cases for it so we could have it.

@aagonzales
Copy link
Member

aagonzales commented Nov 14, 2019

IMO Tags by default should not have a max width, they should just keep growing until the edge of the container.

However, tags should also never be multi-lined so if a tag is constrained by space then it should elipse with a tooltip to show the full content.

image

@aagonzales
Copy link
Member

aagonzales commented Nov 14, 2019

I can really only see this being a problem with user generated tags. Any system tags shouldn't have to ellipse, designers should be putting tags in layout/content area's that allow for whatever the tag widths are needed or designers should consider alternatives for displaying this information if size constraints is problematic.

@skaparelos1
Copy link

Hello,
in case you need data to help your decision: our team's case (IBM team) falls in the user-generated tags bucket. We are using data taken from an API we don't own that could be of variable size. Also the data we get from the API is subject to changing at some point.

@aagonzales
Copy link
Member

@skaparelos1 why is adding an elipses better than letting the tag be displayed at full width? At what max size were you thinking this tags will elipse?

Can you also share a screenshot or where these tags are being used so we can understand the content? Thanks!

@skaparelos1
Copy link

skaparelos1 commented Nov 18, 2019

why is adding an elipses better than letting the tag be displayed at full width?

If the user selected a location like South Georgia and the South Sandwich Islands this would need a lot of space even on a big screen. What if the user is using a mobile phone?

At what max size were you thinking this tags will elipse?

Ideally I would like this to depend on the size of the parent. If it doesn't fit in the parent, then add ellipsis. If it does, then show it all.

Can you also share a screenshot or where these tags are being used so we can understand the content?

I am not sure if I am allowed to post a screenshot here. These tags are being used as filter refinements for a search application. Center of page contains search results whereas on the left side users are given the option to select several options to further refine their search.

@aagonzales
Copy link
Member

Ok let's add the elipse function but the tag itself shouldn't have a max width on it (if that makes since). I think we'll need to be careful on the design side when this can be used.

@jnm2377 jnm2377 removed their request for review November 18, 2019 18:59
@dakahn dakahn removed their request for review November 18, 2019 23:06
@joshblack joshblack requested a review from a team as a code owner December 4, 2019 22:51
@ghost ghost requested review from dakahn and joshblack December 4, 2019 22:51
@joshblack
Copy link
Contributor

bump @dakahn what are the 2.1 requirements for ellipses/truncated text? Anything we need to consider here for ellipses in tags?

@dakahn
Copy link
Contributor

dakahn commented Dec 4, 2019

I can't find a reference to any requirements around truncated text. Usability is a concern, but as long as we've addressed those it seems like Checkpoint 1.4.4 relates to text truncation/rewrap on resize. 👍

@joshblack joshblack merged commit 3118b69 into carbon-design-system:master Dec 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cv-tag filter breaks when using text with length bigger than the width of the tag