feat(tile): add tile component by christiemolloy · Pull Request #3229 · patternfly/patternfly

christiemolloy · 2020-06-30T21:02:22Z

patternfly-build · 2020-06-30T21:06:30Z

Preview: https://patternfly-pr-3229.surge.sh

A11y report: https://patternfly-pr-3229-coverage.surge.sh

maryshak1996 · 2020-07-01T14:24:10Z

@mcarrano initial design specs request using all-black variant of logos on default and disabled, then switch to the full-color logo variant on hover and selected, but seeing this implemented with the full-color logo always used, I think that this still provides enough differentiation between the different states and I would support this as well, what do you think (before I approve this)

^ screenshot of the implementation vs having the black PF logo on default and disabled

maryshak1996

@christiemolloy you'll see I posed a question for Matt as well, but the only thing missing here is that this large stacked title needs to swap to blue for the icon on hover and selected (see here it's black still)

mcarrano

@maryshak1996 I agree with you that allowing the logo to remain full-color in some cases is fine. I can see instances where changing a logotype to all black might to work. I'll defer to you on other issues. But looks good to me.

christiemolloy · 2020-07-06T13:56:39Z

@maryshak1996 I actually saw this in the Marvel mock up so I thought that the larger tiles had a black icon for all states:

But updated to this now:

Also I didn't update the image color on different states, because that would require having that image as a background image, and i think the switch in images would be better solved in React but I can definitely try to achieve that if thats what we still want?

maryshak1996 · 2020-07-06T14:22:12Z

@christiemolloy ahhh my bad there! Looks great now!

mattnolting

Looking great, left a few comments.

mattnolting · 2020-07-09T00:59:21Z

+      --pf-c-tile__icon--FontSize: var(--pf-c-tile__title--m-stacked__icon--FontSize);
+      --pf-c-tile__icon--Color: var(--pf-c-tile__title--m-stacked__icon--Color);
+
+      &.pf-m-large {


Should we use .pf-m-lg or .pf-m-display-lg here?

mattnolting · 2020-07-09T01:12:34Z

+  --pf-c-tile--disabled__subtext--Color: var(--pf-global--disabled-color--100);
+
+  position: relative;
+  display: inline-block;


I think I'd use display: inline-grid here, then apply a row-gap. That way, in the future, if we want to use subgrid to manage row height in the title or text when tiles are adjacent, we could do that.

mattnolting · 2020-07-09T01:16:47Z

+  text-align: center;
+  background-color: var(--pf-c-tile--BackgroundColor);
+
+  &::after {


Suggested change

&::after {

&::before {

We should try to use ::before first before using the ::after pseudo, as ::after always render above the element's content and ::before will naturally render behind content.

mattnolting · 2020-07-09T01:22:34Z

+  --pf-c-tile__title--m-stacked__icon--m-large--FontSize: var(--pf-global--icon--FontSize--xl);
+  --pf-c-tile__title--m-stacked__icon--m-large--Width: var(--pf-c-tile__title--m-stacked__icon--m-large--FontSize);
+  --pf-c-tile__title--m-stacked__icon--m-large--Height: var(--pf-c-tile__title--m-stacked__icon--m-large--Width);


Probably should use .pf-m-lg or .pf-m-display-lg to conform to existing patterns.

mattnolting · 2020-07-09T01:25:22Z

+    --pf-c-tile__title--Color: var(--pf-c-tile--disabled__title--Color);
+    --pf-c-tile__subtext--Color: var(--pf-c-tile--disabled__subtext--Color);
+
+    &::after {


Suggested change

&::after {

&::before {

mattnolting · 2020-07-09T01:27:24Z

+    &::after {
+      --pf-c-tile--BorderWidth: var(--pf-c-tile--m-selected--BorderWidth);
+      --pf-c-tile--BorderColor: var(--pf-c-tile--m-selected--BorderColor);
+    }


Suggested change

&::after {

--pf-c-tile--BorderWidth: var(--pf-c-tile--m-selected--BorderWidth);

--pf-c-tile--BorderColor: var(--pf-c-tile--m-selected--BorderColor);

}

&::before {

--pf-c-tile--before--BorderWidth: var(--pf-c-tile--m-selected--before--BorderWidth);

--pf-c-tile--before--BorderColor: var(--pf-c-tile--m-selected--before--BorderColor);

}

mattnolting · 2020-07-09T01:29:14Z

+  --pf-c-tile--BorderColor: var(--pf-global--BorderColor--100);
+  --pf-c-tile--BorderWidth: var(--pf-global--BorderWidth--sm);
+  --pf-c-tile--BorderRadius: var(--pf-global--BorderRadius--sm);
+  --pf-c-tile--hover--BorderColor: var(--pf-global--primary-color--100);
+  --pf-c-tile--m-selected--BorderWidth: var(--pf-global--BorderWidth--md);
+  --pf-c-tile--m-selected--BorderColor: var(--pf-global--primary-color--100);
+  --pf-c-tile--disabled--BackgroundColor: var(--pf-global--disabled-color--300);


Suggested change

--pf-c-tile--BorderColor: var(--pf-global--BorderColor--100);

--pf-c-tile--BorderWidth: var(--pf-global--BorderWidth--sm);

--pf-c-tile--BorderRadius: var(--pf-global--BorderRadius--sm);

--pf-c-tile--hover--BorderColor: var(--pf-global--primary-color--100);

--pf-c-tile--m-selected--BorderWidth: var(--pf-global--BorderWidth--md);

--pf-c-tile--m-selected--BorderColor: var(--pf-global--primary-color--100);

--pf-c-tile--disabled--BackgroundColor: var(--pf-global--disabled-color--300);

--pf-c-tile--before--BorderColor: var(--pf-global--BorderColor--100);

--pf-c-tile--before--BorderWidth: var(--pf-global--BorderWidth--sm);

--pf-c-tile--before--BorderRadius: var(--pf-global--BorderRadius--sm);

--pf-c-tile--hover--before--BorderColor: var(--pf-global--primary-color--100);

--pf-c-tile--m-selected--before--BorderWidth: var(--pf-global--BorderWidth--md);

--pf-c-tile--m-selected--before--BorderColor: var(--pf-global--primary-color--100);

--pf-c-tile--disabled--before--BackgroundColor: var(--pf-global--disabled-color--300);

--pf-c-tile--disabled--before--BorderWidth: 0;

mattnolting · 2020-07-09T01:31:21Z

+    &::after {
+      --pf-c-tile--BorderColor: var(--pf-c-tile--hover--BorderColor);
+    }


Suggested change

&::after {

--pf-c-tile--BorderColor: var(--pf-c-tile--hover--BorderColor);

}

&::before {

--pf-c-tile--before--BorderColor: var(--pf-c-tile--hover--before--BorderColor);

}

mattnolting · 2020-07-09T01:56:51Z

+    left: 0;
+    content: "";
+    border: var(--pf-c-tile--BorderWidth) solid var(--pf-c-tile--BorderColor);
+    border-radius: var(--pf-c-tile--BorderRadius);


pointer-events: none;

mcoker · 2020-07-09T20:29:56Z

+  }
+
+  // stylelint-disable
+  &[disabled] {


disabled isn't valid on a div - needs to be .pf-m-disabled

mcoker · 2020-07-09T20:31:19Z

+    pointer-events: none;
+    cursor: not-allowed;
+
+    --pf-c-tile--BackgroundColor: var(--pf-c-tile--disabled--BackgroundColor);


can you move the vars to the beginning of the block? We usually list it in the order of vars -> properties -> selectors

mcoker · 2020-07-09T20:32:43Z

+    --pf-c-tile__body--Color: var(--pf-c-tile--disabled__body--Color);
+
+    &::before {
+      border: 0;


you could just set --pf-c-tile--before--BorderWidth: 0; on the disabled (or .pf-m-disabled) selector

mcoker · 2020-07-09T20:33:47Z

+  }
+
+  &:hover {
+    &::before {


do you need this selector? Could you just assign the var under &:hover?

mcoker · 2020-07-09T20:33:59Z

+
+    cursor: pointer;
+
+    --pf-c-tile__title--Color: var(--pf-c-tile--hover__title--Color);


can you move the vars to the beginning of the block?

mcoker · 2020-07-09T20:40:50Z

+
+        img {
+          width: 100%;
+          height: 100%;


this can create a skewed image, should be max-width/height, but what if it's an svg? We could just start with supporting icon fonts and react-icons SVGs, which will accept the font-size declaration, then create a follow-up to investigate other image formats.

mcoker · 2020-07-09T20:41:06Z

+
+.pf-c-tile__header {
+  display: flex;
+  flex-direction: row;


shouldn't need this since it's the default.

mcoker · 2020-07-09T20:43:20Z

+.pf-c-tile__header {
+  display: flex;
+  flex-direction: row;
+  align-items: center;


This will center align the text and icon if the text wraps. Is that what we want to happen?

ill need to follow-up with design on this

mcoker · 2020-07-09T20:56:13Z

+  &.pf-m-selected,
+  &:focus {
+    &::before {
+      --pf-c-tile--before--BorderWidth: var(--pf-c-tile--before--m-selected--BorderWidth);


vars should be title--m-selected--before--

mcoker · 2020-07-09T21:19:20Z

-    }
-
    .pf-c-tile__header.pf-m-stacked .pf-c-tile__icon {
      --pf-c-tile__icon--Color: var(--pf-c-tile--disabled__header--m-stacked__icon--Color);


does this need to be set on .pf-c-tile__header.pf-m-stacked .pf-c-tile__icon or could we just set it under .pf-m-disabled as --pf-c-tile__icon--Color: var(--pf-c-tile--m-disabled__header__icon--Color);

mcoker · 2020-07-09T21:21:05Z

+    --pf-c-tile--before--BorderWidth: var(--pf-c-tile--m-selected--before--BorderWidth);
+    --pf-c-tile--before--BorderColor: var(--pf-c-tile--m-selected--before--BorderColor);
+
+    cursor: pointer;


could cursor: pointer just be set on the entire component? The card sets .pf-m-selectable { cursor: pointer; }

mattnolting · 2020-07-09T21:13:47Z

+| `.pf-c-tile__body` | `<div>` | Initiates the tile body. |
+| `.pf-m-selected` | `.pf-c-tile` | Modifies the tile for the selected state. |
+| `.pf-m-stacked` | `.pf-c-tile__header` | Modifies the tile header to be stacked vertically. |
+| `.pf-m-display-lg` | `.pf-c-tile` | Modifies the tile to have large display styling. |


mattnolting · 2020-07-09T21:15:54Z

+  --pf-c-tile--before--m-selected--BorderWidth: var(--pf-global--BorderWidth--md);
+  --pf-c-tile--before--m-selected--BorderColor: var(--pf-global--primary-color--100);


Suggested change

--pf-c-tile--before--m-selected--BorderWidth: var(--pf-global--BorderWidth--md);

--pf-c-tile--before--m-selected--BorderColor: var(--pf-global--primary-color--100);

--pf-c-tile--m-selected--before--BorderWidth: var(--pf-global--BorderWidth--md);

--pf-c-tile--m-selected--before--BorderColor: var(--pf-global--primary-color--100);

mattnolting · 2020-07-09T21:17:01Z

+  --pf-c-tile__title--Color: var(--pf-global--Color--100);
+  --pf-c-tile--hover__title--Color: var(--pf-global--primary-color--100);
+  --pf-c-tile--m-selected__title--Color: var(--pf-global--primary-color--100);
+  --pf-c-tile--disabled__title--Color: var(--pf-global--disabled-color--100);


Suggested change

--pf-c-tile--disabled__title--Color: var(--pf-global--disabled-color--100);

--pf-c-tile--m-disabled__title--Color: var(--pf-global--disabled-color--100);

mattnolting · 2020-07-09T21:17:43Z

+  --pf-c-tile__header--m-stacked__icon--Color: var(--pf-global--Color--100);
+  --pf-c-tile--hover__header--m-stacked__icon--Color: var(--pf-global--primary-color--100);
+  --pf-c-tile--m-selected__header--m-stacked__icon--Color: var(--pf-global--primary-color--100);
+  --pf-c-tile--disabled__header--m-stacked__icon--Color: var(--pf-global--Color--200);


Suggested change

--pf-c-tile--disabled__header--m-stacked__icon--Color: var(--pf-global--Color--200);

--pf-c-tile--m-disabled__header--m-stacked__icon--Color: var(--pf-global--Color--200);

mattnolting · 2020-07-09T21:17:58Z

+  // body
+  --pf-c-tile__body--Color: var(--pf-global--Color--100);
+  --pf-c-tile__body--FontSize: var(--pf-global--FontSize--xs);
+  --pf-c-tile--disabled__body--Color: var(--pf-global--disabled-color--100);


Suggested change

--pf-c-tile--disabled__body--Color: var(--pf-global--disabled-color--100);

--pf-c-tile--m-disabled__body--Color: var(--pf-global--disabled-color--100);

mattnolting · 2020-07-09T21:18:27Z

+    pointer-events: none;
+    cursor: not-allowed;
+
+    --pf-c-tile--BackgroundColor: var(--pf-c-tile--disabled--BackgroundColor);


Suggested change

--pf-c-tile--BackgroundColor: var(--pf-c-tile--disabled--BackgroundColor);

--pf-c-tile--BackgroundColor: var(--pf-c-tile--m-disabled--BackgroundColor);

mattnolting · 2020-07-09T21:18:46Z

+    --pf-c-tile__title--Color: var(--pf-c-tile--disabled__title--Color);
+    --pf-c-tile__body--Color: var(--pf-c-tile--disabled__body--Color);


Suggested change

--pf-c-tile__title--Color: var(--pf-c-tile--disabled__title--Color);

--pf-c-tile__body--Color: var(--pf-c-tile--disabled__body--Color);

--pf-c-tile__title--Color: var(--pf-c-tile--m-disabled__title--Color);

--pf-c-tile__body--Color: var(--pf-c-tile--m-disabled__body--Color);

mattnolting · 2020-07-09T21:19:03Z

+    }
+
+    .pf-c-tile__header.pf-m-stacked .pf-c-tile__icon {
+      --pf-c-tile__icon--Color: var(--pf-c-tile--disabled__header--m-stacked__icon--Color);


Suggested change

--pf-c-tile__icon--Color: var(--pf-c-tile--disabled__header--m-stacked__icon--Color);

--pf-c-tile__icon--Color: var(--pf-c-tile--m-disabled__header--m-stacked__icon--Color);

mattnolting · 2020-07-09T21:21:32Z

+    flex-direction: column;
+    justify-content: initial;
+
+    .pf-c-tile__icon {


Should you use the same pattern we use w/button in pf-m-start/end?

I think design only calls for it to be on the left

mcoker · 2020-07-09T21:23:06Z

      --pf-c-tile__icon--Color: var(--pf-c-tile__header--m-stacked__icon--Color);
+
+      display: flex;
+      justify-content: center;


you probably want to add align-items: center; here, or at least on the display-lg.stacked icon selector since that has a defined height and the image may be shorter than the height

mcoker · 2020-07-09T21:50:10Z

    --pf-c-tile__title--Color: var(--pf-c-tile--m-disabled__title--Color);
    --pf-c-tile__body--Color: var(--pf-c-tile--m-disabled__body--Color);
    --pf-c-tile--before--BorderWidth: 0;
+    --pf-c-tile__icon--Color: var(--pf-c-tile--m-disabled__header--m-stacked__icon--Color);


could this be renamed?

Suggested change

--pf-c-tile__icon--Color: var(--pf-c-tile--m-disabled__header--m-stacked__icon--Color);

--pf-c-tile__icon--Color: var(--pf-c-tile--m-disabled__icon--Color);

mattnolting

It's pretty much perfect!

christiemolloy added 2 commits June 30, 2020 11:54

feat(tile): add tile component

a235b2d

fix(tile): update tile component

0234a36

fix(tile): more updates

85041cc

christiemolloy requested review from maryshak1996, mattnolting, mcarrano and mcoker July 1, 2020 13:50

maryshak1996 suggested changes Jul 1, 2020

View reviewed changes

mcarrano reviewed Jul 1, 2020

View reviewed changes

fix(tile): update visuals

d867c49

maryshak1996 self-requested a review July 6, 2020 14:22

maryshak1996 approved these changes Jul 6, 2020

View reviewed changes

mattnolting reviewed Jul 9, 2020

View reviewed changes

fix(tile): fix tile from review

cf05cb0

mcoker reviewed Jul 9, 2020

View reviewed changes

fix(tile): update

995b4d6

mcoker reviewed Jul 9, 2020

View reviewed changes

mattnolting reviewed Jul 9, 2020

View reviewed changes

mcoker reviewed Jul 9, 2020

View reviewed changes

fix(tile): review from coker and nolting

65db258

mcoker reviewed Jul 9, 2020

View reviewed changes

christiemolloy and others added 3 commits July 9, 2020 18:01

fix(tile): update

0d83dec

fix(tile): update disabled

ce940f5

fix(tile): reorder vars

89a4fd8

mcoker approved these changes Jul 9, 2020

View reviewed changes

chore(tile): update stylelint rule

ed138b0

mattnolting approved these changes Jul 9, 2020

View reviewed changes

mattnolting merged commit 664d2e1 into patternfly:master Jul 9, 2020

christiemolloy mentioned this pull request Jul 10, 2020

fix(tile): remove support for imgs #3274

Merged

mcoker mentioned this pull request Jan 12, 2021

fix(card): introduce large card variant #3793

Merged

-  --pf-c-tile--BorderColor: var(--pf-global--BorderColor--100);
-  --pf-c-tile--BorderWidth: var(--pf-global--BorderWidth--sm);
-  --pf-c-tile--BorderRadius: var(--pf-global--BorderRadius--sm);
-  --pf-c-tile--hover--BorderColor: var(--pf-global--primary-color--100);
-  --pf-c-tile--m-selected--BorderWidth: var(--pf-global--BorderWidth--md);
-  --pf-c-tile--m-selected--BorderColor: var(--pf-global--primary-color--100);
-  --pf-c-tile--disabled--BackgroundColor: var(--pf-global--disabled-color--300);
+  --pf-c-tile--before--BorderColor: var(--pf-global--BorderColor--100);
+  --pf-c-tile--before--BorderWidth: var(--pf-global--BorderWidth--sm);
+  --pf-c-tile--before--BorderRadius: var(--pf-global--BorderRadius--sm);
+  --pf-c-tile--hover--before--BorderColor: var(--pf-global--primary-color--100);
+  --pf-c-tile--m-selected--before--BorderWidth: var(--pf-global--BorderWidth--md);
+  --pf-c-tile--m-selected--before--BorderColor: var(--pf-global--primary-color--100);
+  --pf-c-tile--disabled--before--BackgroundColor: var(--pf-global--disabled-color--300);
+  --pf-c-tile--disabled--before--BorderWidth: 0;


		cursor: pointer;

		--pf-c-tile__title--Color: var(--pf-c-tile--hover__title--Color);

		--pf-c-tile--before--m-selected--BorderWidth: var(--pf-global--BorderWidth--md);
		--pf-c-tile--before--m-selected--BorderColor: var(--pf-global--primary-color--100);

	--pf-c-tile--disabled__title--Color: var(--pf-global--disabled-color--100);
	--pf-c-tile--m-disabled__title--Color: var(--pf-global--disabled-color--100);

	--pf-c-tile--disabled__header--m-stacked__icon--Color: var(--pf-global--Color--200);
	--pf-c-tile--m-disabled__header--m-stacked__icon--Color: var(--pf-global--Color--200);

	--pf-c-tile--disabled__body--Color: var(--pf-global--disabled-color--100);
	--pf-c-tile--m-disabled__body--Color: var(--pf-global--disabled-color--100);

	--pf-c-tile--BackgroundColor: var(--pf-c-tile--disabled--BackgroundColor);
	--pf-c-tile--BackgroundColor: var(--pf-c-tile--m-disabled--BackgroundColor);

		--pf-c-tile__title--Color: var(--pf-c-tile--disabled__title--Color);
		--pf-c-tile__body--Color: var(--pf-c-tile--disabled__body--Color);

	--pf-c-tile__icon--Color: var(--pf-c-tile--disabled__header--m-stacked__icon--Color);
	--pf-c-tile__icon--Color: var(--pf-c-tile--m-disabled__header--m-stacked__icon--Color);

Conversation

christiemolloy commented Jun 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patternfly-build commented Jun 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maryshak1996 commented Jul 1, 2020

Uh oh!

maryshak1996 left a comment

Choose a reason for hiding this comment

Uh oh!

mcarrano left a comment

Choose a reason for hiding this comment

Uh oh!

christiemolloy commented Jul 6, 2020

Uh oh!

maryshak1996 commented Jul 6, 2020

Uh oh!

mattnolting left a comment

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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christiemolloy commented Jun 30, 2020 •

edited

Loading

patternfly-build commented Jun 30, 2020 •

edited

Loading

mattnolting Jul 9, 2020 •

edited

Loading