Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(Card): updated v5 logic to prevent unclickable cards #10202

Merged
merged 5 commits into from
Mar 28, 2024
Merged

fix(Card): updated v5 logic to prevent unclickable cards #10202

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
989894a
fix(Card): updated v5 logic to prevent unclickable cards
thatblindgeye Mar 26, 2024
8bfd525
Updated API to have isSelected control selectable card state
thatblindgeye Mar 28, 2024
2974cb0
Updated cypress test
thatblindgeye Mar 28, 2024
322c154
Updated cypress test
thatblindgeye Mar 28, 2024
9d90581
Updated cypress test
thatblindgeye Mar 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 24 additions & 7 deletions packages/react-core/src/components/Card/Card.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,21 @@ export interface CardProps extends React.HTMLProps<HTMLElement>, OUIAProps {
component?: keyof JSX.IntrinsicElements;
/** Modifies the card to include compact styling. Should not be used with isLarge. */
isCompact?: boolean;
/** Modifies the card to include selectable styling */
/** Flag indicating that the card is selectable. */
isSelectable?: boolean;
/** @deprecated Specifies the card is selectable, and applies raised styling on hover and select */
isSelectableRaised?: boolean;
/** Modifies the card to include selected styling */
isSelected?: boolean;
/** Modifies the card to include clickable styling */
/** Flag indicating that the card is clickable and contains some action that triggers on click. */
isClickable?: boolean;
/** Modifies a clickable or selectable card to have disabled styling. */
/** Flag indicating whether a card that is either selectable only or both clickable and selectable is
* currently selected and has selected styling.
*/
isSelected?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

We may want to deprecate this prop and come up with a better name. The prop not determining the selected state is confusing given the name.

Copy link
Contributor Author

@thatblindgeye thatblindgeye Mar 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

True. We could keep this as is and I can update the logic so that isSelected determines the isChecked value within CardHeader (essentially what I'm doing with the isClicked prop for "clickable" cards). Then we can either set the isChecked value for the checkbox/radio in the card based on either the CardHeader's selectableActions isChecked prop value or Card's isSelected value.

That would just require deprecating the isChecked prop in CardHeader's CardHeaderSelectableActionsObject interface. Doing that would be a little more consistent with the API with this update; rather than needing to pass a prop in CardHeader tor selectable cards and a prop on Card for clickable cards. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that sounds like it would be more intuitive for the consumer.

/** Flag indicating whether a card that is either only clickable or that is both clickable and selectable
* is currently clicked and has clicked styling.
*/
isClicked?: boolean;
/** Flag indicating that a clickable or selectable card is disabled. */
isDisabled?: boolean;
/** @deprecated Modifies a raised selectable card to have disabled styling */
isDisabledRaised?: boolean;
Expand Down Expand Up @@ -56,6 +62,8 @@ interface CardContextProps {
isExpanded: boolean;
isClickable: boolean;
isSelectable: boolean;
isSelected: boolean;
isClicked: boolean;
isDisabled: boolean;
// TODO: Remove hasSelectableInput when deprecated prop is removed
hasSelectableInput: boolean;
Expand All @@ -72,6 +80,8 @@ export const CardContext = React.createContext<Partial<CardContextProps>>({
isExpanded: false,
isClickable: false,
isSelectable: false,
isSelected: false,
isClicked: false,
isDisabled: false
});

Expand All @@ -86,6 +96,7 @@ export const Card: React.FunctionComponent<CardProps> = ({
isDisabled = false,
isSelectableRaised = false,
isSelected = false,
isClicked = false,
isDisabledRaised = false,
isFlat = false,
isExpanded = false,
Expand Down Expand Up @@ -119,15 +130,19 @@ export const Card: React.FunctionComponent<CardProps> = ({
return css(styles.modifiers.selectableRaised, isSelected && styles.modifiers.selectedRaised);
}
if (isSelectable && isClickable) {
return css(styles.modifiers.selectable, styles.modifiers.clickable, isSelected && styles.modifiers.current);
return css(
styles.modifiers.selectable,
styles.modifiers.clickable,
(isSelected || isClicked) && styles.modifiers.current
);
}

if (isSelectable) {
return css(styles.modifiers.selectable, isSelected && styles.modifiers.selected);
}

if (isClickable) {
return css(styles.modifiers.clickable, isSelected && styles.modifiers.selected);
return css(styles.modifiers.clickable, isClicked && styles.modifiers.current);
}

return '';
Expand Down Expand Up @@ -162,6 +177,8 @@ export const Card: React.FunctionComponent<CardProps> = ({
isExpanded,
isClickable,
isSelectable,
isSelected,
isClicked,
isDisabled,
// TODO: Remove hasSelectableInput when deprecated prop is removed
hasSelectableInput
Expand Down
21 changes: 11 additions & 10 deletions packages/react-core/src/components/Card/CardHeader.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,9 @@ export interface CardHeaderSelectableActionsObject {
isExternalLink?: boolean;
/** Name for the input element of a clickable or selectable card. */
name?: string;
/** Flag indicating whether the selectable card input is checked */
/** @deprecated Flag indicating whether the selectable card input is checked. We recommend using
* the isSelected prop on the card component instead.
*/
isChecked?: boolean;
}

Expand Down Expand Up @@ -80,7 +82,7 @@ export const CardHeader: React.FunctionComponent<CardHeaderProps> = ({
}: CardHeaderProps) => (
<CardContext.Consumer>
{/* TODO: Remove hasSelectableInput when deprecated props are removed */}
{({ cardId, isClickable, isSelectable, isDisabled: isCardDisabled, hasSelectableInput }) => {
{({ cardId, isClickable, isSelectable, isSelected, isClicked, isDisabled: isCardDisabled, hasSelectableInput }) => {
const cardHeaderToggle = (
<div className={css(styles.cardHeaderToggle)}>
<Button
Expand Down Expand Up @@ -112,12 +114,10 @@ export const CardHeader: React.FunctionComponent<CardHeaderProps> = ({
}

const handleActionClick = (event: React.FormEvent<HTMLInputElement> | React.MouseEvent) => {
if (isClickable) {
if (selectableActions?.onClickAction) {
selectableActions.onClickAction(event);
} else if (selectableActions?.to) {
window.open(selectableActions.to, selectableActions.isExternalLink ? '_blank' : '_self');
}
if (selectableActions?.onClickAction) {
selectableActions.onClickAction(event);
} else if (selectableActions?.to) {
window.open(selectableActions.to, selectableActions.isExternalLink ? '_blank' : '_self');
}
};

Expand All @@ -132,12 +132,13 @@ export const CardHeader: React.FunctionComponent<CardHeaderProps> = ({
name: selectableActions.name,
isDisabled: isCardDisabled
};
const isSelectableInputChecked = selectableActions.isChecked ?? isSelected;

if (isClickable && !isSelectable) {
return { ...baseProps, onClick: handleActionClick };
return { ...baseProps, onClick: handleActionClick, isChecked: isClicked };
}
if (isSelectable) {
return { ...baseProps, onChange: selectableActions.onChange, isChecked: selectableActions.isChecked };
return { ...baseProps, onChange: selectableActions.onChange, isChecked: isSelectableInputChecked };
}

return baseProps;
Expand Down
20 changes: 11 additions & 9 deletions packages/react-core/src/components/Card/examples/Card.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,37 +109,39 @@ A common use case of this is to set all but one body section to `isFilled={false

### Selectable

A card can be selected by clicking anywhere within the card.
A selectable card can be selected by clicking anywhere within the card.

You must avoid rendering any other interactive content within the `<Card>` when it is meant to be selectable only. Refer to our [clickable and selectable example](#clickable-and-selectable-cards) if you need a card that is both selectable and has other interactive content.
You must avoid rendering any other interactive content within the `<Card>` when it is meant to be selectable only. Refer to our [actionable and selectable example](#clickable-and-selectable-cards) if you need a card that is both selectable and has other interactive, actionable content.

```ts file='./CardSelectable.tsx'

```

### Single selectable

When a group of single selectable cards are related, you should pass the same `name` property to each card's `selectableActions` property.
When a group of single selectable cards are related, you must pass the same `name` property to each card's `selectableActions` property.

```ts file='./CardSingleSelectable.tsx'

```

### Clickable
### Actionable

A card can perform an action or navigate to a link by clicking anywhere within the card. You can also pass in the `isExternalLink` property to `selectableActions` if you want a clickable card's link to open in a new tab or window.
An actionable card can perform an action or navigate to a link by clicking anywhere within the card. To open a link in a new tab or window, pass the `isExternalLink` property to `selectableActions`.

When a card is meant to be clickable only, you must avoid rendering any other interactive content within the `<Card>`, similar to selectable cards.
You can pass the `isClicked` property to `<Card>` to convey that a card is the currently clicked one, such as when clicking a card would open a [primary-detail view](/patterns/primary-detail). This must not be used simply for "selection" of a card, and you should instead use our [selectable card](#selectable) or [single selectable card](#single-selectable).

When a card is meant to be actionable only, you must avoid rendering any other interactive content within the `<Card>`, similar to selectable cards.

```ts file='./CardClickable.tsx'

```

### Clickable and selectable
### Actionable and selectable

A card can be selectable and have additional interactive content by passing both the `isClickable` and `isSelectable` properties to `<Card>`. The following example shows how the "clickable" functionality can be rendered anywhere within a selectable card.
A card can be selectable and have additional interactive content by passing both the `isClickable` and `isSelectable` properties to `<Card>`. The following example shows how the actionable functionality can be rendered anywhere within a selectable card.

When passing interactive content to a clickable and selectable card that is disabled, you should also ensure the interactive content is disabled as well, if applicable.
When passing interactive content to an actionable and selectable card that is disabled, you should also ensure the interactive content is disabled as well, if applicable.

```ts file='./CardClickableSelectable.tsx'

Expand Down
19 changes: 11 additions & 8 deletions packages/react-core/src/components/Card/examples/CardClickableSelectable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,14 @@ export const CardClickable: React.FunctionComponent = () => {
const [isChecked1, setIsChecked1] = React.useState(false);
const [isChecked2, setIsChecked2] = React.useState(false);
const [isChecked3, setIsChecked3] = React.useState(false);
const [isSelected1, setIsSelected1] = React.useState(false);
const [isClicked, setIsClicked] = React.useState(false);

const id1 = 'clickable-selectable-card-input-1';
const id2 = 'clickable-selectable-card-input-2';
const id3 = 'clickable-selectable-card-input-3';

const onClick = () => {
setIsSelected1((prevState) => !prevState);
setIsClicked((prevState) => !prevState);
};

const onChange = (event: React.FormEvent<HTMLInputElement>, checked: boolean) => {
Expand All @@ -33,13 +33,18 @@ export const CardClickable: React.FunctionComponent = () => {

return (
<React.Fragment>
<Card id="clickable-selectable-card-example-1" isClickable isSelectable isSelected={isSelected1}>
<Card
id="clickable-selectable-card-example-1"
isClickable
isClicked={isClicked}
isSelectable
isSelected={isChecked1}
>
<CardHeader
selectableActions={{
selectableActionId: id1,
selectableActionAriaLabelledby: 'clickable-selectable-card-example-1',
name: id1,
isChecked: isChecked1,
onChange
}}
>
Expand All @@ -51,13 +56,12 @@ export const CardClickable: React.FunctionComponent = () => {
</CardHeader>
<CardBody>This card performs an action upon clicking the card title and is selectable.</CardBody>
</Card>
<Card id="clickable-selectable-card-example-2" isClickable isSelectable>
<Card id="clickable-selectable-card-example-2" isClickable isSelectable isSelected={isChecked2}>
<CardHeader
selectableActions={{
selectableActionId: id2,
selectableActionAriaLabelledby: 'clickable-selectable-card-example-2',
name: id2,
isChecked: isChecked2,
onChange
}}
>
Expand All @@ -71,13 +75,12 @@ export const CardClickable: React.FunctionComponent = () => {
.
</CardBody>
</Card>
<Card id="clickable-selectable-card-example-3" isClickable isSelectable isDisabled>
<Card id="clickable-selectable-card-example-3" isClickable isSelectable isDisabled isSelected={isChecked3}>
<CardHeader
selectableActions={{
selectableActionId: id3,
selectableActionAriaLabelledby: 'clickable-selectable-card-example-3',
name: id3,
isChecked: isChecked3,
onChange
}}
>
Expand Down
9 changes: 3 additions & 6 deletions packages/react-core/src/components/Card/examples/CardSelectable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,41 +28,38 @@ export const SelectableCard: React.FunctionComponent = () => {

return (
<React.Fragment>
<Card id="selectable-card-example-1" isSelectable>
<Card id="selectable-card-example-1" isSelectable isSelected={isChecked1}>
<CardHeader
selectableActions={{
selectableActionId: id1,
selectableActionAriaLabelledby: 'selectable-card-example-1',
name: id1,
isChecked: isChecked1,
onChange
}}
>
<CardTitle>First card</CardTitle>
</CardHeader>
<CardBody>This card is selectable.</CardBody>
</Card>
<Card id="selectable-card-example-2" isSelectable>
<Card id="selectable-card-example-2" isSelectable isSelected={isChecked2}>
<CardHeader
selectableActions={{
selectableActionId: id2,
selectableActionAriaLabelledby: 'selectable-card-example-2',
name: id2,
isChecked: isChecked2,
onChange
}}
>
<CardTitle>Second card</CardTitle>
</CardHeader>
<CardBody>This card is selectable.</CardBody>
</Card>
<Card id="selectable-card-example-3" isSelectable isDisabled>
<Card id="selectable-card-example-3" isSelectable isDisabled isSelected={isChecked3}>
<CardHeader
selectableActions={{
selectableActionId: id3,
selectableActionAriaLabelledby: 'selectable-card-example-3',
name: id3,
isChecked: isChecked3,
onChange
}}
>
Expand Down
20 changes: 14 additions & 6 deletions packages/react-core/src/components/Card/examples/CardSingleSelectable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,45 +2,53 @@ import React from 'react';
import { Card, CardHeader, CardTitle, CardBody } from '@patternfly/react-core';

export const SingleSelectableCard: React.FunctionComponent = () => {
const [isChecked, setIsChecked] = React.useState('');
const id1 = 'single-selectable-card-input-1';
const id2 = 'single-selectable-card-input-2';
const id3 = 'single-selectable-card-input-3';

const onChange = (event: React.FormEvent<HTMLInputElement>) => {
setIsChecked(event.currentTarget.id);
};

return (
<React.Fragment>
<Card id="single-selectable-card-example-1" isSelectable>
<Card id="single-selectable-card-example-1" isSelectable isSelected={isChecked === id1}>
<CardHeader
selectableActions={{
selectableActionId: id1,
selectableActionAriaLabelledby: 'single-selectable-card-example-1',
name: 'single-selectable-card-example',
variant: 'single'
variant: 'single',
onChange
}}
>
<CardTitle>First card</CardTitle>
</CardHeader>
<CardBody>This card is single selectable.</CardBody>
</Card>
<Card id="single-selectable-card-example-2" isSelectable>
<Card id="single-selectable-card-example-2" isSelectable isSelected={isChecked === id2}>
<CardHeader
selectableActions={{
selectableActionId: id2,
selectableActionAriaLabelledby: 'single-selectable-card-example-2',
name: 'single-selectable-card-example',
variant: 'single'
variant: 'single',
onChange
}}
>
<CardTitle>Second card</CardTitle>
</CardHeader>
<CardBody>This card is single selectable.</CardBody>
</Card>
<Card id="single-selectable-card-example-3" isSelectable isDisabled>
<Card id="single-selectable-card-example-3" isSelectable isDisabled isSelected={isChecked === id3}>
<CardHeader
selectableActions={{
selectableActionId: id3,
selectableActionAriaLabelledby: 'single-selectable-card-example-3',
name: 'single-selectable-card-example',
variant: 'single'
variant: 'single',
onChange
}}
>
<CardTitle>Third card</CardTitle>
Expand Down
2 changes: 0 additions & 2 deletions packages/react-integration/cypress/integration/card.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,10 +72,8 @@ describe('Card Demo Test', () => {

it('Verify clickable only card action is triggered', () => {
cy.get('#clickable-card-drawer').should('not.have.class', 'pf-m-expanded');
cy.get('#clickable-card-example-1 #clickable-card-input-1').should('not.be.checked');
cy.get('#clickable-card-example-1').click();
cy.get('#clickable-card-drawer').should('have.class', 'pf-m-expanded');
cy.get('#clickable-card-example-1 #clickable-card-input-1').should('be.checked');
});

it('Verify clickable only card link is navigated to', () => {
Expand Down
Loading
Loading