feat(Card): Add disabled card and clean up props by nicolethoen · Pull Request #6659 · patternfly/patternfly-react

nicolethoen · 2021-11-30T21:37:55Z

What: Closes #6567

patternfly-build · 2021-11-30T21:48:00Z

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

mcoker

LGTM! Just one question.

mcoker · 2021-12-01T00:50:36Z

    return (
      <React.Fragment>
-        <Card isHoverable>
+        <Card isSelectable selectableVariant="raised">


Just a thought - could we combine these so you don't need separate props? Wondering if isSelectable="raised" could apply the new modifier.

This is a good thought - I figured that adding the selectableVariant="raised" would mean that to opt into the new styling, they'd just need to add indicate they wanted to using the new prop. And eventually, when raised is the default, then it wont be necessary to specify that.

mcarrano

Looks great. Thanks @nicolethoen !

mcoker

🥳

wise-king-sullyman · 2021-12-01T23:01:03Z

+  let selectableModifiers = '';
+  if (isSelectable || isHoverable) {
+    if (isRaised) {
+      if (isDisabled) {
+        selectableModifiers = css((isSelectable || isHoverable) && styles.modifiers.nonSelectableRaised);
+      } else {
+        selectableModifiers = css(
+          (isSelectable || isHoverable) && styles.modifiers.selectableRaised,
+          (isSelectable || isHoverable) && isSelected && styles.modifiers.selectedRaised
+        );
+      }
+    } else {
+      selectableModifiers = css(
+        (isSelectable || isHoverable) && styles.modifiers.selectable,
+        (isSelectable || isHoverable) && isSelected && styles.modifiers.selected
+      );
+    }
+  }


I feel like this would be cleaner as a function rather than mutating a let.

wise-king-sullyman · 2021-12-01T23:01:14Z

+        selectableModifiers = css((isSelectable || isHoverable) && styles.modifiers.nonSelectableRaised);
+      } else {
+        selectableModifiers = css(
+          (isSelectable || isHoverable) && styles.modifiers.selectableRaised,
+          (isSelectable || isHoverable) && isSelected && styles.modifiers.selectedRaised
+        );
+      }
+    } else {
+      selectableModifiers = css(
+        (isSelectable || isHoverable) && styles.modifiers.selectable,
+        (isSelectable || isHoverable) && isSelected && styles.modifiers.selected


Also, I don't think the repeated isSelectable || isHoverable conditionals here are needed as they are already nested within an if checking that conditional, so they will always be true. I could be missing something though.

tlabaj · 2021-12-02T16:16:53Z

-  /** @beta Modifies the card to include selected-raised styling */
-  isSelectedRaised?: boolean;
+  /** @beta Specifies the selectable styling variant */
+  selectableVariant?: 'legacy' | 'raised';


legacy implies the non raised variant will eventually be deprecated. is that the case @mcarrano?

I figured it means it's not preferred and the 'old but supported' way

Ok I see my confusion. I thought the selectable variant set the card to selectable. That being said. I think the API is a little confusing. May we can rename the prop tp selectableStylingVariant.
I would also suggest not calling the option legacy. It is not really descriptive.

So I was thinking about this more. Would it make sense to deprecate the isSelectable prop and let the selectableVariant prop not only set the styling, but set it selectable too? I think that would make the API more straight forward. Similar to what @mcoker was suggesting in his comment about combining props.

How about selectStyle or selectableStyle over using variant, since the function of the select remains the same?
Edit: variant was removed nvm

tlabaj · 2021-12-02T16:21:53Z


  it('Verify that selectable card can be selected and unselected with keyboard input', () => {
    cy.get('#selectableCard').focus();
-    cy.focused().should('have.class', 'pf-m-selectable');


So non raised card can no longer be selected or selectable?

correct - I think it's the same as disabled from my understanding

I don't think that is the case. We are still applying the style and we have an example for that use case. We should be testing both here.

tlabaj

I have a few questions about the logic.

tlabaj · 2021-12-02T19:57:22Z

  /** Modifies the card to include compact styling. Should not be used with isLarge. */
  isCompact?: boolean;
  /** Modifies the card to include selectable styling */
  isSelectable?: boolean;


so, is this prop deprecated?

Are we going to keep supporting the original plain variation of selectable cards? If not I'd say the next breaking release we update isSelectable to use the raised styling and remove isSelectableRaised (I'm just not sure if that really falls under the "deprecated" flag indication though). If we will keep the original styling I think we should bring back in the enum for the style variations and keep using isSelectable as is.

Yes. I was confused at first. I thought the original variant prop also set the card to be selectable.

tlabaj · 2021-12-02T20:16:34Z

-  isSelectedRaised?: boolean;
+  /** @beta Specifies the selectable styling variant */
+  selectableVariant?: 'legacy' | 'raised';
+  /** @beta Modifies a selectable card to have disabled styling */


Can we be more explicit here that disabled only applies to non raised cards since we still support both variations.

As I have it, a disabled selectable card without the raised variant is basically a normal card with no hover/select styles at all. Still valid?

Yeah I think only the disabled raised selectable card has any special styling. The flag probably shouldn't apply anything to a non-selectable non-raised card (or alternatively, maybe remove any select related props from non-raised cards?)

kmcfaul · 2021-12-02T21:48:58Z

Code looks good to me the only outstanding question I've got is whether we want to keep supporting the original plain selectable style, as that would impact how we mark isSelectable and whether we want an enum prop or bool prop for the raised styling flag.

@mcarrano

mcoker · 2021-12-02T22:02:42Z

@kmcfaul I think it would be a breaking change if we didn't support it.

mcoker

LGTM! A thought about isDisabled - I wonder if we'll introduce a "disabled" card at some point that does something else, and if that might create a naming conflict. I don't know that we would, but figured I would mention it.

tlabaj · 2021-12-03T15:20:14Z

        id={id}
        className={css(
          styles.card,
-          isHoverable && styles.modifiers.hoverable,


@mcoker, could someone apply pf-m-hoverable. Without, isSelectable. I think removing support completely would technically be breaking. I think we can deprecate it, but we still need to apply the style.

@tlabaj yep, they're separate. While you could use them together, it isn't necessary since isSelected has its own hoverable styles (they're the same as what isHoverable applies). https://github.com/patternfly/patternfly/blob/dc6cf07d69cbb80026dca7fe43dc28704aabea64/src/patternfly/components/Card/card.scss#L26-L27

mcarrano · 2021-12-03T15:28:46Z

Code looks good to me the only outstanding question I've got is whether we want to keep supporting the original plain selectable style, as that would impact how we mark isSelectable and whether we want an enum prop or bool prop for the raised styling flag.

@nicolethoen Yes, we still need to support the old selectable style. We didn't want to force a visual breaking change on products currently using selectable cards.

tlabaj · 2021-12-06T14:29:55Z

@mcarrano is the intention that next time we have a breaking change release, the old styles will go away?

mcarrano · 2021-12-06T14:34:32Z

@mcarrano is the intention that next time we have a breaking change release, the old styles will go away?

Yes, most likely.

tlabaj · 2021-12-06T15:04:50Z

+
+  if (isHoverable) {
+    // eslint-disable-next-line no-console
+    console.warn("Card: the 'isHoverable' prop is deprecated. Use isSelectable instead.");


I don't thinks this is necessary. We don't do this anywhere else that I can find.

tlabaj

LGTM

patternfly-build · 2021-12-06T19:32:23Z

Your changes have been released in:

eslint-plugin-patternfly-react@4.18.0
@patternfly/react-catalog-view-extension@4.30.0
@patternfly/react-charts@6.32.0
@patternfly/react-code-editor@4.20.0
@patternfly/react-console@4.30.0
@patternfly/react-core@4.179.0
@patternfly/react-docs@5.40.0
@patternfly/react-icons@4.30.0
@patternfly/react-inline-edit-extension@4.24.0
demo-app-ts@4.139.0
@patternfly/react-integration@4.141.0
@patternfly/react-log-viewer@4.24.0
@patternfly/react-styles@4.29.0
@patternfly/react-table@4.48.0
@patternfly/react-tokens@4.31.0
@patternfly/react-topology@4.26.0
@patternfly/react-virtualized-extension@4.26.0
transformer-cjs-imports@4.17.0

Thanks for your contribution! 🎉

feat(Card): Add disabled card and clean up props

b4761db

convert my selectable example to ts

874391e

nicolethoen requested review from mcarrano, mcoker, tlabaj and wise-king-sullyman December 1, 2021 00:31

mcoker reviewed Dec 1, 2021

View reviewed changes

update catalog view tile and snapshots

c7787e2

mcarrano approved these changes Dec 1, 2021

View reviewed changes

mcoker approved these changes Dec 1, 2021

View reviewed changes

wise-king-sullyman reviewed Dec 1, 2021

View reviewed changes

clean up modifier logic

4808c9d

tlabaj reviewed Dec 2, 2021

View reviewed changes

tlabaj requested changes Dec 2, 2021

View reviewed changes

tlabaj reviewed Dec 2, 2021

View reviewed changes

nicolethoen added 2 commits December 2, 2021 16:22

remove variant and simplify it all into isSelectable/isSelectableRaised

c9515ca

update isDisabled description

3626464

wise-king-sullyman approved these changes Dec 2, 2021

View reviewed changes

mcoker approved these changes Dec 2, 2021

View reviewed changes

update tests and change isDisabled to isDisabledRaised

917bd68

tlabaj reviewed Dec 3, 2021

View reviewed changes

update prop in example

4b02c32

tlabaj reviewed Dec 6, 2021

View reviewed changes

remove warning and update prop description

3dd956f

tlabaj approved these changes Dec 6, 2021

View reviewed changes

tlabaj merged commit 86f225a into patternfly:main Dec 6, 2021

nicolethoen deleted the non_selectable_card branch February 8, 2023 13:48

Conversation

nicolethoen commented Nov 30, 2021

Uh oh!

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

Uh oh!

Uh oh!

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

mcarrano left a comment

Choose a reason for hiding this comment

Uh oh!

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

tlabaj Dec 2, 2021 • 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!

kmcfaul Dec 2, 2021 • 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!

tlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmcfaul Dec 2, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

kmcfaul commented Dec 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcoker commented Dec 2, 2021

Uh oh!

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

mcarrano commented Dec 3, 2021

Uh oh!

tlabaj commented Dec 6, 2021

Uh oh!

mcarrano commented Dec 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patternfly-build commented Nov 30, 2021 •

edited

Loading

tlabaj Dec 2, 2021 •

edited

Loading

kmcfaul Dec 2, 2021 •

edited

Loading

kmcfaul Dec 2, 2021 •

edited

Loading

kmcfaul commented Dec 2, 2021 •

edited

Loading