improvement(CatalogTile): Removed truncation fade and fixed length by rebeccaalpert · Pull Request #3378 · patternfly/patternfly-react · GitHub

Removed truncation fade per OpenShift request and added CSS/SCSS to deal with the description length (limit to 3 lines maximum).

lgtm

PatternFly-React preview: https://patternfly-react-pr-3378.surge.sh

lgtm

I disagree with the 3 line strategy. It won't be suitable for all applications and will leave tiles truncated with trailing empty vertical space. Better than what is there now, but only because it currently leaves an abundance of empty space.

Are we against using a height measurer as was done at https://github.com/patternfly/patternfly-react/blob/master/packages/patternfly-3/patternfly-react-extensions/src/components/CatalogTile/CatalogTile.js#L29 ?

co?

Codecov Report

Merging #3378 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3378      +/-   ##
==========================================
- Coverage    67.1%   67.09%   -0.01%     
==========================================
  Files         903      903              
  Lines       25483    25482       -1     
  Branches     2254     2257       +3     
==========================================
- Hits        17100    17097       -3     
- Misses       7342     7343       +1     
- Partials     1041     1042       +1

Flag	Coverage Δ
#misc	95.45% <ø> (ø)	⬆️
#patternfly3	69.29% <ø> (ø)	⬆️
#patternfly4	64.19% <100%> (-0.02%)	⬇️

Impacted Files	Coverage Δ
...tension/src/components/CatalogTile/CatalogTile.tsx	90.19% <100%> (-4.04%)	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4574ad2...f9f40e3. Read the comment docs.

So the reason we have so much empty vertical space in OpenShift right now is to accommodate the absolute longest tile. This new change will keep everything a more consistent length so we don't need as much space (we will potentially need some vertical space given that titles and providers vary so much in length -- that's something we could potentially do this with as well if Sam is game for it).

This is patternfly-react, not specific to Console, right?

Yes, this is PatternFly-React rather than a console-specific change. We've been using OpenShift as a staging area to see how this component behaves with more variable data.

If we are auto-truncating via the co-line-clamp then we should not be truncating the string at all and no need for the truncated class.

We should remove the truncation properties (and update the text in the example tiles)

The original catalog tiles had a truncation function and allowed you to pass one in. Removing it would be a breaking change; is that what you want to do?

I worked with Joe on a refs-based solution. We have to do more complicated stuff here since it's not a fixed width and height container, but it seems to work. I know you're on vacation, but let us know your thoughts when you get back.

lgtm after updating the examples

some console statements still in code fyi (in case you didn't realize)

Thanks Joe.

lgtm

LGTM, a few suggestions which would be nice to get in.

👍

This should not be on the exported type, only on the inner component.

This should not be exported.

I don't believe this is necessary.

I still maintain that a hard coded line truncation is a bad idea.

Hey - I wound up speaking to Dana about this yesterday because I was noticing some performance issues with the refs on the PF4 developer site. He and Michael Coker suggested we use a CSS solution, so I'm in the process of rolling back to the original approach. Coker thought line clamp was a decent approach for this.

Serena just pointed out the issue we've been trying to fix with this PR to a larger audience and Joe and I would like to fast-track this fix so we can get it in under the wire into this upcoming version of OpenShift.

Why is this less performant than it was in the previous version?

In addition to the issues with the line height that we discussed over Slack, there seemed to be an issue where we were intermittently receiving the incorrect clientHeight. It happened intermittently on refresh on the dev site. I asked Dana about it and we tried running a production build to see if it was an issue with the dev site. However, on the production build, you would see a delay, where there was an incorrect clientHeight and then the correct values seemed to kick in, resizing the content. This is not acceptable behavior for OpenShift.

Do all items in the catalog have icons? If you don't have one it looks strange since the header is so large.

They mostly have text badges. We're going to shrink them down in OpenShift to widths and heights that make sense for those layouts.

…st and added CSS to better trun

Rebased.

lgtm

lgtm thanks for updating the snapshots

jcaianirh previously approved these changes Dec 5, 2019

View reviewed changes

rebeccaalpert dismissed jcaianirh’s stale review via dca40c0 December 5, 2019 15:23

jcaianirh previously approved these changes Dec 5, 2019

View reviewed changes

jeff-phillips-18 reviewed Dec 5, 2019

View reviewed changes

rebeccaalpert dismissed jcaianirh’s stale review via a112809 December 18, 2019 20:35

jcaianirh previously approved these changes Dec 19, 2019

View reviewed changes

rebeccaalpert dismissed jcaianirh’s stale review via 1c5a1d1 December 19, 2019 16:25

jcaianirh suggested changes Dec 19, 2019

View reviewed changes

jcaianirh previously approved these changes Jan 2, 2020

View reviewed changes

jeff-phillips-18 reviewed Jan 6, 2020

View reviewed changes

Comment thread packages/patternfly-4/react-catalog-view-extension/src/components/CatalogTile/CatalogTile.tsx Outdated

Comment thread packages/patternfly-4/react-catalog-view-extension/src/components/CatalogTile/CatalogTile.tsx Outdated

rebeccaalpert dismissed jcaianirh’s stale review via 60f8106 January 7, 2020 19:11

jeff-phillips-18 previously approved these changes Jan 8, 2020

View reviewed changes

rebeccaalpert dismissed jeff-phillips-18’s stale review via c700810 January 8, 2020 20:18

jeff-phillips-18 previously approved these changes Jan 8, 2020

View reviewed changes

rebeccaalpert dismissed jeff-phillips-18’s stale review via f7912e1 January 8, 2020 20:46

jeff-phillips-18 reviewed Jan 9, 2020

View reviewed changes

dlabaj reviewed Jan 10, 2020

View reviewed changes

rebeccaalpert and others added 12 commits January 13, 2020 11:24

improvement(CatalogTile): Removed truncation fade per OpenShift reque…

307b647

…st and added CSS to better trun

Fix test

9550053

Truncate more when footer; rename class

27a1926

Add refs

85e3d8d

update calculations to compute height

06742e8

Added refs to control description height more programmatically

1eb09fa

Update examples

fde74cd

Update tests

a7ecc9a

Remove console statements

9236c1b

Update ref handling

16933a8

Address PR feedback

71e6c8f

Revert to CSS-based solution as refs were not performant

394623b

dlabaj previously approved these changes Jan 13, 2020

View reviewed changes

jcaianirh previously approved these changes Jan 13, 2020

View reviewed changes

Update snapshot

f9f40e3

rebeccaalpert dismissed stale reviews from jcaianirh and dlabaj via f9f40e3 January 13, 2020 21:37

jcaianirh approved these changes Jan 13, 2020

View reviewed changes

dlabaj approved these changes Jan 14, 2020

View reviewed changes

dlabaj merged commit d6e9156 into patternfly:master Jan 14, 2020

                       <CardBody className="catalog-tile-pf-body">
                         <div className="catalog-tile-pf-description">
-                          <span className={classNames({'truncated': isTruncated})}>
+                          <span className={classNames('co-line-clamp', {'truncated': isTruncated})}>

                 /** Modifies the card to include compact styling */
                 isCompact?: boolean;
+                /** Callback for card ref */
+                innerRef?: React.Ref<any>;

               }
-              export const Card: React.FunctionComponent<CardProps> = ({
+              export const Card0: React.FunctionComponent<CardProps> = ({

                 );
               };
+              // eslint-disable-next-line react/no-multi-comp

		@@ -1,3 +1,10 @@
		@mixin co-line-clamp($lines: 3) {

improvement(CatalogTile): Removed truncation fade and fixed length by rebeccaalpert · Pull Request #3378 · patternfly/patternfly-react · GitHub

Uh oh!

improvement(CatalogTile): Removed truncation fade and fixed length - #3378

improvement(CatalogTile): Removed truncation fade and fixed length#3378dlabaj merged 13 commits intopatternfly:masterpatternfly/patternfly-react:masterfrom rebeccaalpert:catalogtile-openshift-updatesCopy head branch name to clipboard

Conversation

rebeccaalpert commented Dec 5, 2019

Uh oh!

jcaianirh left a comment

Choose a reason for hiding this comment

Uh oh!

patternfly-build commented Dec 5, 2019

Uh oh!

jcaianirh left a comment

Choose a reason for hiding this comment

Uh oh!

jeff-phillips-18 left a comment

Choose a reason for hiding this comment

Uh oh!

jeff-phillips-18 Dec 5, 2019

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Dec 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rebeccaalpert commented Dec 5, 2019

Uh oh!

jeff-phillips-18 commented Dec 5, 2019

Uh oh!

rebeccaalpert commented Dec 5, 2019

Uh oh!

jeff-phillips-18 Dec 5, 2019

Choose a reason for hiding this comment

Uh oh!

rebeccaalpert commented Dec 18, 2019

Uh oh!

jcaianirh 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!

jcaianirh left a comment

Choose a reason for hiding this comment

Uh oh!

rebeccaalpert commented Jan 2, 2020

Uh oh!

jcaianirh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jeff-phillips-18 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jeff-phillips-18 left a comment

Choose a reason for hiding this comment

Uh oh!

jeff-phillips-18 Jan 9, 2020

Choose a reason for hiding this comment

Uh oh!

jeff-phillips-18 Jan 9, 2020

Choose a reason for hiding this comment

Uh oh!

jeff-phillips-18 Jan 9, 2020

Choose a reason for hiding this comment

Uh oh!

jeff-phillips-18 commented Jan 10, 2020

Uh oh!

rebeccaalpert commented Jan 10, 2020

Uh oh!

jeff-phillips-18 commented Jan 10, 2020

Uh oh!

rebeccaalpert commented Jan 10, 2020

Uh oh!

dlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

rebeccaalpert commented Jan 10, 2020

improvement(CatalogTile): Removed truncation fade and fixed length#3378
dlabaj merged 13 commits into
patternfly:masterfrom
rebeccaalpert:catalogtile-openshift-updates

codecov-io commented Dec 5, 2019 •
edited
Loading

jcaianirh left a comment •
edited
Loading