feat(Label): truncate by default #8771

gitdallas · 2023-03-02T20:23:41Z

What: Closes #8258

Labels: https://patternfly-react-pr-8771.surge.sh/components/label

patternfly-build · 2023-03-02T20:32:21Z

Preview: https://patternfly-react-pr-8771.surge.sh

A11y report: https://patternfly-react-pr-8771-a11y.surge.sh

mcoker

Discussed offline, but left a comment about

removing isTruncated
adding a prop for a custom truncation length

Also these conditionals should no longer be needed. We can just include the top block - the span.pf-c-label__text wrapping {children}

patternfly-react/packages/react-core/src/components/Label/Label.tsx

Lines 211 to 216 in 40e7ec3

    
           {isTruncated && ( 
        
             <span ref={textRef} className={css(styles.labelText)}> 
        
               {children} 
        
             </span> 
        
           )} 
        
           {!isTruncated && children}

packages/react-core/src/components/Label/Label.tsx

mcoker

Just a couple of test updates that I didn't catch before, otherwise LGTM!

packages/react-core/src/components/Label/__tests__/Label.test.tsx

nicolethoen

is there no default value for the --pf-c-label__text--MaxWidth variable? this PR is named 'truncate by default' but if it needs textMaxWidth prop, then it's not default?

gitdallas · 2023-03-03T18:55:54Z

@nicolethoen - i believe it could still truncate due to other constraints limiting the size. @mcoker thoughts?

mcarrano

Why are the removable and non-removable labels in the examples displaying at different heights?

gitdallas · 2023-03-03T18:59:06Z

@mcarrano - there is another PR to address label/chip restructure, i could rebase this after that one is merged in

mattnolting · 2023-03-03T19:01:31Z

Why are the removable and non-removable labels in the examples displaying at different heights?

@mcarrano bc the global reset (which was removed) adds padding: 0 margin: 0 to [class*=pf-c-], which applies to buttons. There are buttons inside of those labels.

gitdallas · 2023-03-03T19:11:13Z

@mcarrano @mattnolting - here's the PR for that: #8773

i'll rebase this on that once it gets merged.

thatblindgeye

Other than a non-blocker below and other comments that have been left, only other thing is whether it'd be worth adding a label to show the default truncation without textMaxWidth passed in. That also isn't a blocker, though.

packages/react-core/src/components/Label/Label.tsx

mcoker · 2023-03-03T19:30:46Z

@nicolethoen - i believe it could still truncate due to other constraints limiting the size. @mcoker thoughts?

Correct, it will truncate by default now when it runs out of space in its container. Setting the textMaxWidth prop allows for a custom width.

mcarrano

This looks good now. I agree with @thatblindgeye that it would be nice to have an example that explicitly shows the effect of adding a maxWidth, but not a blocker.

mcoker

one small nit on a test

mcoker · 2023-03-03T21:25:05Z

packages/react-core/src/components/Label/__tests__/Label.test.tsx

@@ -86,7 +86,7 @@ describe('Label', () => {

  test('label with truncation', () => {
    const { asFragment } = render(
-      <Label isTruncated>Something very very very very very long that should be truncated</Label>
+      <Label textMaxWidth="16c">textMaxWidth set at 16h on this very long label that should be truncated</Label>


Suggested change

<Label textMaxWidth="16c">textMaxWidth set at 16h on this very long label that should be truncated</Label>

<Label textMaxWidth="16ch">textMaxWidth set at 16ch on this very long label that should be truncated</Label>

tlabaj · 2023-03-03T21:29:04Z

packages/react-core/src/components/Label/Label.tsx

@@ -90,7 +90,7 @@ export const Label: React.FunctionComponent<LabelProps> = ({
  isCompact = false,
  isEditable = false,
  editableProps,
-  isTruncated = false,
+  textMaxWidth = '',


Should this be undefined?

well line 218 is a falsy check, but i don't see why we couldn't just let this be undefined

gitdallas · 2023-03-03T21:37:05Z

@mcarrano @thatblindgeye - sorry i must have missed that comment, added an example of a parent-constrained label https://patternfly-react-pr-8771.surge.sh/components/label/

tlabaj

LGTM

patternfly-build · 2023-03-04T03:26:05Z

Your changes have been released in:

@patternfly/react-code-editor@5.0.0-alpha.25
@patternfly/react-core@5.0.0-alpha.25
@patternfly/react-docs@6.0.0-alpha.28
demo-app-ts@5.0.0-alpha.8
@patternfly/react-integration@5.0.0-alpha.4
@patternfly/react-table@5.0.0-alpha.25

Thanks for your contribution! 🎉

gitdallas requested review from jenny-s51, nicolethoen and mcoker March 2, 2023 21:21

mcoker requested changes Mar 2, 2023

View reviewed changes

packages/react-core/src/components/Label/Label.tsx Outdated Show resolved Hide resolved

tlabaj added the css review label Mar 2, 2023

gitdallas requested a review from mcoker March 3, 2023 04:22

mcoker requested changes Mar 3, 2023

View reviewed changes

packages/react-core/src/components/Label/__tests__/Label.test.tsx Outdated Show resolved Hide resolved

packages/react-core/src/components/Label/__tests__/Label.test.tsx Outdated Show resolved Hide resolved

tlabaj requested a review from mmenestr March 3, 2023 18:20

tlabaj added the ux review label Mar 3, 2023

tlabaj requested a review from mcarrano March 3, 2023 18:20

mattnolting self-requested a review March 3, 2023 18:29

mattnolting approved these changes Mar 3, 2023

View reviewed changes

packages/react-core/src/components/Label/__tests__/Label.test.tsx Outdated Show resolved Hide resolved

nicolethoen reviewed Mar 3, 2023

View reviewed changes

mcarrano requested changes Mar 3, 2023

View reviewed changes

thatblindgeye approved these changes Mar 3, 2023

View reviewed changes

packages/react-core/src/components/Label/Label.tsx Outdated Show resolved Hide resolved

gitdallas and others added 9 commits March 3, 2023 14:29

feat(Label): truncate by default

714ef6a

update snaps

d8a406d

remove unnecessary isTruncated props, add isTruncated false test

b401383

fix(label): truncation, test updates

b5ce214

build

a337f77

build after gh incident

898e6dc

update snap

b978c44

fix label integration tests

0a92e19

address feedback, rebase

10ce0f8

gitdallas force-pushed the feat/8258-labels-truncate-default branch from 026d986 to 10ce0f8 Compare March 3, 2023 20:34

gitdallas requested review from mcoker and mcarrano March 3, 2023 20:35

thatblindgeye approved these changes Mar 3, 2023

View reviewed changes

nicolethoen approved these changes Mar 3, 2023

View reviewed changes

mcarrano approved these changes Mar 3, 2023

View reviewed changes

add parent constrained label

aa45edb

mcoker approved these changes Mar 3, 2023

View reviewed changes

tlabaj requested changes Mar 3, 2023

View reviewed changes

gitdallas added 2 commits March 3, 2023 15:34

pr feedback

2e07668

update snap

1955f41

fix grammar

cfd675e

tlabaj approved these changes Mar 3, 2023

View reviewed changes

nicolethoen merged commit df52d2d into patternfly:v5 Mar 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Label): truncate by default #8771

feat(Label): truncate by default #8771

gitdallas commented Mar 2, 2023 •

edited

patternfly-build commented Mar 2, 2023 •

edited

mcoker left a comment

mcoker left a comment

nicolethoen left a comment

gitdallas commented Mar 3, 2023

mcarrano left a comment

gitdallas commented Mar 3, 2023 •

edited

mattnolting commented Mar 3, 2023 •

edited

gitdallas commented Mar 3, 2023 •

edited

thatblindgeye left a comment

mcoker commented Mar 3, 2023

mcarrano left a comment

mcoker left a comment

mcoker Mar 3, 2023

tlabaj Mar 3, 2023 •

edited

gitdallas Mar 3, 2023

gitdallas commented Mar 3, 2023

tlabaj left a comment

patternfly-build commented Mar 4, 2023

	{isTruncated && (
	<span ref={textRef} className={css(styles.labelText)}>
	{children}
	</span>
	)}
	{!isTruncated && children}

	<Label textMaxWidth="16c">textMaxWidth set at 16h on this very long label that should be truncated</Label>
	<Label textMaxWidth="16ch">textMaxWidth set at 16ch on this very long label that should be truncated</Label>

feat(Label): truncate by default #8771

feat(Label): truncate by default #8771

Conversation

gitdallas commented Mar 2, 2023 • edited

patternfly-build commented Mar 2, 2023 • edited

mcoker left a comment

Choose a reason for hiding this comment

mcoker left a comment

Choose a reason for hiding this comment

nicolethoen left a comment

Choose a reason for hiding this comment

gitdallas commented Mar 3, 2023

mcarrano left a comment

Choose a reason for hiding this comment

gitdallas commented Mar 3, 2023 • edited

mattnolting commented Mar 3, 2023 • edited

gitdallas commented Mar 3, 2023 • edited

thatblindgeye left a comment

Choose a reason for hiding this comment

mcoker commented Mar 3, 2023

mcarrano left a comment

Choose a reason for hiding this comment

mcoker left a comment

Choose a reason for hiding this comment

mcoker Mar 3, 2023

Choose a reason for hiding this comment

tlabaj Mar 3, 2023 • edited

Choose a reason for hiding this comment

gitdallas Mar 3, 2023

Choose a reason for hiding this comment

gitdallas commented Mar 3, 2023

tlabaj left a comment

Choose a reason for hiding this comment

patternfly-build commented Mar 4, 2023

gitdallas commented Mar 2, 2023 •

edited

patternfly-build commented Mar 2, 2023 •

edited

gitdallas commented Mar 3, 2023 •

edited

mattnolting commented Mar 3, 2023 •

edited

gitdallas commented Mar 3, 2023 •

edited

tlabaj Mar 3, 2023 •

edited