FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix(CatalogTile): Edit target selector by rebeccaalpert · Pull Request #3291 · patternfly/patternfly-react · GitHub

fix(CatalogTile): Edit target selector - #3291

Merged
jeff-phillips-18 merged 1 commit into
patternfly:masterfrom
rebeccaalpert:catalogtileedits
Nov 11, 2019
Merged

fix(CatalogTile): Edit target selector#3291
jeff-phillips-18 merged 1 commit into
patternfly:masterfrom
rebeccaalpert:catalogtileedits

Conversation

Copy link
Copy Markdown
Member

Adjusted selector for gradient effect.

Copy link
Copy Markdown
Collaborator

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

codecov-io commented Nov 8, 2019
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #3291 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3291   +/-   ##
=======================================
  Coverage   67.44%   67.44%           
=======================================
  Files         892      892           
  Lines       24867    24867           
  Branches     2141     2141           
=======================================
  Hits        16772    16772           
  Misses       7091     7091           
  Partials     1004     1004
Flag Coverage Δ
#misc 95.45% <ø> (ø) ⬆️
#patternfly3 69.3% <ø> (ø) ⬆️
#patternfly4 64.78% <ø> (ø) ⬆️

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 03d0285...7b6d67b. Read the comment docs.

jcaianirh previously approved these changes Nov 8, 2019

jcaianirh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

position: absolute;
right: 0;
width: 50%;
text-align: right;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Better to have:

.catalog-tile-pf-description {
 .truncated {
    overflow: hidden;
    position: relative;

    &::after {
      background: linear-gradient(to right, rgba(255, 255, 255, 0), rgba(255, 255, 255, 1) 75%);
      bottom: 0;
      color: transparent;
      content: ".";
      position: absolute;
      right: 0;
      width: 50%;
      text-align: right;
    }
  }
}

However, I don't believe we want this only applied when truncated is set. The idea is for this to auto truncate when necessary and the caller need not provide truncated text.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The class is applied whenever the truncation function is called. The user doesn't have to provide truncated text.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Updated the commit with the change.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Why the check for isTruncated at all?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

If all the descriptions have these styles, even really short descriptions that don't need truncation get faded out. This is a due to a difference between the old card and the new card. The old card had a fixed height applied based on the amount of space the text took up, so it wasn't an issue. I didn't think it was a great idea to use the fixed height thing with PF4 card styles.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

OK, this PR is fine by me. The height issue might be a problem if applications try to show tiles with differing header text lengths (causing wrapping). In PF3, that was taken into consideration and the height allowed for the descriptive text auto adjusted for it. Something to watch out for.

Adjusted selector for gradient effect.

Copy link
Copy Markdown
Collaborator

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

jcaianirh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

jeff-phillips-18 merged commit 16bf30c into patternfly:master Nov 11, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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.

5 participants


Back | FazBrowse Home | New Git URL