feat(Truncate): added Truncate component by gabipodolnikova · Pull Request #6713 · patternfly/patternfly-react

gabipodolnikova · 2021-12-22T13:38:22Z

What: Closes #6566

patternfly-build · 2021-12-22T13:47:44Z

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

jpuzz0 · 2021-12-22T17:16:52Z

A couple styling questions that might be more for patternfly core to address:
Should users be able to drag the truncation extender icon beyond container elements?

Should we consider adding some padding between the end of the content and the extender icon?

Should we consider applying a minimum width in some cases?

jpuzz0 · 2021-12-22T14:40:48Z

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

+  end: styles.truncateStart
+};
+
+interface TruncateProps extends React.HTMLProps<HTMLDivElement> {


Looks like this should be extending HTMLProps from HTMLSpanElement instead.

Yes I originally had div there and then changed it and forgot to do this...

jpuzz0 · 2021-12-22T14:43:02Z

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

+  /** Class to add to outer span */
+  className?: string;
+  /** Text to truncate */
+  content: string;


Wondering if we considered just using children for content instead. I don't have much of a preference, but curious about your thoughts.

I think that if we know that the content will always be a string, there is no need to have it as children and just use a prop content. But this was one thing that I knew that someone will be commenting, so totally up for debate :D

I wonder if at some point we'd want content to be bold or italicized. In that case, I think we'd want to just use ReactNode. If there's potential for that, I think it's worth a debate, or at least the changing of the type, otherwise if we know it will always be a string, what you have is fine with me.

This is a good question for @mattnolting
Will the css that controls the truncation still work if someone wants to put an icon or link somewhere in the string?

The text can be bold or italicized, you can just add those tags around. But with the icon it won't work. So I think that it is best we keep it as string.

gabipodolnikova · 2022-01-03T12:52:28Z

A couple styling questions that might be more for patternfly core to address: Should users be able to drag the truncation extender icon beyond container elements?

Should we consider adding some padding between the end of the content and the extender icon?

Should we consider applying a minimum width in some cases?

I used the same styling as it is in core. So this is definitely a question for maybe @mattnolting ? :)

mcarrano

This is looking good @gabipodolnikova . I also had a question about the drag/resize icon and capability. Is that actually part of the component or is it part of the example to let the user see how truncation works? If the former, I don't think we'd want to allow that in all cases. In most places where truncation is used, there is not space to make the field wider.

Also, in either case, it would be good to introduce some padding between the icon and string.

gabipodolnikova · 2022-01-04T15:38:37Z

This is looking good @gabipodolnikova . I also had a question about the drag/resize icon and capability. Is that actually part of the component or is it part of the example to let the user see how truncation works? If the former, I don't think we'd want to allow that in all cases. In most places where truncation is used, there is not space to make the field wider.

Also, in either case, it would be good to introduce some padding between the icon and string.

It is not a part of the component, it is just to help to demonstrate how it works. I can add some padding. But the padding should also be added in the core example as I totally used what is in there :) @mattnolting

jpuzz0

Looks good to me other than the styling issues mentioned here, which I assume will be handled outside of this repo if at all.

mattnolting · 2022-01-06T21:15:03Z

It is not a part of the component, it is just to help to demonstrate how it works. I can add some padding. But the padding should also be added in the core example as I totally used what is in there :) @mattnolting

@mcarrano, @gabipodolnikova is right, I should fix up the Core example in a new issue.

tlabaj · 2022-01-06T21:56:27Z

@mattnolting @mcarrano It looks like when the truncation is in the middle, there is sometimes a noticeable large space to the right of the ellipses. On the left, the ellipses are always right next to last character before them. Is this intentional?

mcarrano · 2022-01-07T15:28:06Z

@mattnolting @mcarrano It looks like when the truncation is in the middle, there is sometimes a noticeable large space to the right of the ellipses. On the left, the ellipses are always right next to last character before them. Is this intentional?

This is definitely not intentional. Do you know what's causing that @mattnolting ? Is it just an artifact of the example with a resizable container? Ideally there would be no extra space between the "..." and the second string.

nicolethoen · 2022-01-10T00:28:10Z

should we attach a resize observer to the component parent container to recalculate where to break the strings on resize? to avoid the gap?

nicolethoen · 2022-01-10T19:05:26Z

@gabipodolnikova,
after talking to @mattnolting, he is going to make a core change so there is still no need for a resize observer. disregard my last comment.

If this PR needs an update after the core PR is done, Im happy to update it, too if you'd like. Just let me know.

mattnolting · 2022-01-10T20:42:45Z

@nicolethoen @mcarrano @tlabaj Responses to your comments

gabipodolnikova · 2022-01-11T09:20:46Z

@gabipodolnikova, after talking to @mattnolting, he is going to make a core change so there is still no need for a resize observer. disregard my last comment.

If this PR needs an update after the core PR is done, Im happy to update it, too if you'd like. Just let me know.

Thanks for resolving this for me Nicole and Matt! :D I will update this PR when the core is merged :)

nicolethoen · 2022-01-18T15:28:32Z

@gabipodolnikova The PR was merged! :)
patternfly/patternfly#4599

mcarrano

Looks great. Thanks @gabipodolnikova !

tlabaj

Looks great! thanks Gabi.

jpuzz0 · 2022-01-20T14:30:56Z

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

  className?: string;
  /** Text to truncate */
  content: string;
+  /** Number of characters that shoul be sliced at the end of the content string. Works best if the number is between 5 to 10. */


Some suggested spelling adjustments:
"that should be sliced"
"between 5 and 10"

Also, I'm curious why a number between 5 and 10 would work the best. I'm thinking others might wonder the same thing reading this.

Thanks for the spellcheck... And why numbers between 5 and 10, I am taking it from this PR patternfly/patternfly#4599 I could probably just say that we suggest these numbers... It is not mentioned in the core, just in that PR, so I wanted to mention it somewhere :) I welcome any better formulation of this piece of information.

I think specifying it's a suggestion for the sake of readability makes sense. Maybe the example text which echos the same thing could be updated with that verbiage as well.

1 other suggestion is to rename the prop to something like charGap (or something like gap) to add some more context to what the slicing will do, which is create a gap of whitespace. Not a blocking comment, just figured I'd throw this out there to see what you or others thought.

mattnolting

This looks fantastic, thanks for your hard work and patience with this issue, Gabi!!! Well done 👏👏👏

patternfly-build · 2022-01-20T16:48:49Z

Your changes have been released in:

eslint-plugin-patternfly-react@4.31.0
@patternfly/react-catalog-view-extension@4.43.0
@patternfly/react-charts@6.45.0
@patternfly/react-code-editor@4.33.0
@patternfly/react-console@4.43.0
@patternfly/react-core@4.192.0
@patternfly/react-docs@5.53.0
@patternfly/react-icons@4.43.0
@patternfly/react-inline-edit-extension@4.37.0
demo-app-ts@4.152.0
@patternfly/react-integration@4.154.0
@patternfly/react-log-viewer@4.37.0
@patternfly/react-styles@4.42.0
@patternfly/react-table@4.61.0
@patternfly/react-tokens@4.44.0
@patternfly/react-topology@4.39.0
@patternfly/react-virtualized-extension@4.39.0
transformer-cjs-imports@4.30.0

Thanks for your contribution! 🎉

nicolethoen requested review from jpuzz0, mattnolting, mcarrano, nicolethoen and wise-king-sullyman December 22, 2021 13:42

jpuzz0 reviewed Dec 22, 2021

View reviewed changes

mcarrano reviewed Jan 4, 2022

View reviewed changes

tlabaj added ux review labels Jan 4, 2022

jpuzz0 approved these changes Jan 5, 2022

View reviewed changes

wise-king-sullyman approved these changes Jan 6, 2022

View reviewed changes

mattnolting mentioned this pull request Jan 6, 2022

fix(truncate): update example patternfly/patternfly#4590

Closed

mattnolting mentioned this pull request Jan 10, 2022

fix(truncate): resolved spacing issues patternfly/patternfly#4599

Merged

gabipodolnikova added 5 commits January 18, 2022 17:06

feat(Truncate): Added new component

7d9b92b

Added Tooltip

c2847a4

fixes condition

d407dff

Small review fix

db077e0

Fixes and updates from core

ce2353e

gabipodolnikova force-pushed the new-component-truncate branch from 8705aa9 to ce2353e Compare January 19, 2022 17:45

Fixes tests

a7ae3be

gabipodolnikova requested a review from mcarrano January 20, 2022 10:06

mcarrano approved these changes Jan 20, 2022

View reviewed changes

tlabaj approved these changes Jan 20, 2022

View reviewed changes

jpuzz0 reviewed Jan 20, 2022

View reviewed changes

mattnolting approved these changes Jan 20, 2022

View reviewed changes

Review fixes number I don't even know

2dbca5b

jpuzz0 merged commit c803b5d into patternfly:main Jan 20, 2022

Conversation

gabipodolnikova commented Dec 22, 2021

Uh oh!

patternfly-build commented Dec 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jpuzz0 commented Dec 22, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabipodolnikova commented Jan 3, 2022

Uh oh!

mcarrano left a comment

Choose a reason for hiding this comment

Uh oh!

gabipodolnikova commented Jan 4, 2022

Uh oh!

jpuzz0 left a comment • 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!

mattnolting commented Jan 6, 2022

Uh oh!

tlabaj commented Jan 6, 2022

Uh oh!

mcarrano commented Jan 7, 2022

Uh oh!

nicolethoen commented Jan 10, 2022

Uh oh!

nicolethoen commented Jan 10, 2022

Uh oh!

mattnolting commented Jan 10, 2022

Uh oh!

gabipodolnikova commented Jan 11, 2022

Uh oh!

nicolethoen commented Jan 18, 2022

Uh oh!

mcarrano left a comment

Choose a reason for hiding this comment

Uh oh!

tlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

jpuzz0 Jan 20, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattnolting left a comment

Choose a reason for hiding this comment

Uh oh!

patternfly-build commented Jan 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

patternfly-build commented Dec 22, 2021 •

edited

Loading

jpuzz0 left a comment •

edited

Loading

jpuzz0 Jan 20, 2022 •

edited

Loading