fix(Card): fix toggle-in-card navigation (#165)

* fix: handle onClick in Clickable instead of Card if ToggleItem is rendered If Card renders a Clickable radio toggle, and has tabIndex (onClick prop), it breaks arrow-navigation between other Toggle radio Cards. In order to fix it, the onClick is moved to the Clickable component and name prop is provided to Clickable radios of the same group. * refactor(card): refactor non-flat card style * fix(clickable): add focus-ring and focus-ring-inset classes to ToggleItem Display visual feedback on ToggleItem focus * feat(card): log deprecation warning if Card receives onClick prop Click events should be handled by Clickable component, not Card. Before implementing a breaking change in Card, a deprecation warning is logged if Card receives an onClick prop * test: update tests with the onClick prop changes * refactor(dead-toggle): rename 'name' prop in DeadToggle to avoid confusion
fabric-ds · Nov 9, 2022 · fd03fc6 · fd03fc6
1 parent 4ce9c82
commit fd03fc6
Show file tree

Hide file tree

Showing 11 changed files with 214 additions and 166 deletions.
diff --git a/packages/_helpers/clickable.tsx b/packages/_helpers/clickable.tsx
@@ -37,7 +37,6 @@ export type ClickableProps = {
 
   /**
    * Click handler
-   * If passed, clickable renders as a button with a click event
    */
   onClick?: () => void;
 } & Partial<Omit<HTMLAnchorElement, 'children'>> &
@@ -56,12 +55,12 @@ export function Clickable({
   return radio || checkbox ? (
     <ToggleItem
       labelClassName={props.labelClassName}
-      className="absolute inset-0 h-full w-full appearance-none cursor-pointer focus-ring"
+      className="focus-ring focus-ring-inset absolute inset-0 h-full w-full appearance-none cursor-pointer"
       type={type}
       controlled={false}
-      onChange={() => undefined}
+      onChange={props.onClick ? props.onClick : () => undefined}
       value={value}
-      name={`${id}:toggle`}
+      name={`${props.name || id}:toggle`}
     >
       {children}
     </ToggleItem>

diff --git a/packages/_helpers/dead-toggle.tsx b/packages/_helpers/dead-toggle.tsx
@@ -48,7 +48,7 @@ export function DeadToggle(props: DeadToggleProps) {
         type={type}
         className="hidden"
         labelClassName={props.labelClassName}
-        name="test"
+        name="dead-toggle"
         controlled={true}
         onChange={() => undefined}
         value={props.value}

diff --git a/packages/card/docs/Card.mdx b/packages/card/docs/Card.mdx
@@ -172,7 +172,7 @@ function Example() {
 
   return (
     <div className="space-y-32 md:space-y-0 md:grid grid-cols-3 gap-32">
-      <Card onClick={() => setSelected(!selected)} selected={selected}>
+      <Card selected={selected}>
         <img
           className="h-128 w-full object-cover"
           src="https://source.unsplash.com/random/400x400"
@@ -184,8 +184,10 @@ function Example() {
         <div className="p-16">
           <p className="text-12 text-gray-300">DNB Eiendom</p>
           <p>
-            Stilfull og gjennomgående 3-roms m/balkong. Oppusset i 2019. Inkl.
-            bl.a. vv/fyring.
+            <Clickable checkbox onClick={() => setSelected(!selected)}>
+              Stilfull og gjennomgående 3-roms m/balkong. Oppusset i 2019. Inkl.
+              bl.a. vv/fyring.
+            </Clickable>
           </p>
           <p className="text-14 text-gray-400 mb-4">Bøgata 25C, 0655 Oslo</p>
           <p className="font-bold my-8">
@@ -207,7 +209,7 @@ function Example() {
           </p>
         </div>
       </Card>
-      <Card onClick={() => setSelected(!selected)} selected={selected}>
+      <Card selected={selected}>
         <img
           className="h-128 w-full object-cover"
           src="https://source.unsplash.com/random/403x403"
@@ -216,8 +218,10 @@ function Example() {
         <div className="p-16">
           <p className="text-12 text-gray-300">DNB Eiendom</p>
           <p>
-            Stilfull og gjennomgående 3-roms m/balkong. Oppusset i 2019. Inkl.
-            bl.a. vv/fyring.
+            <Clickable checkbox onClick={() => setSelected(!selected)}>
+              Stilfull og gjennomgående 3-roms m/balkong. Oppusset i 2019. Inkl.
+              bl.a. vv/fyring.
+            </Clickable>
           </p>
           <p className="text-14 text-gray-400 mb-4">Bøgata 25C, 0655 Oslo</p>
           <p className="font-bold my-8">
@@ -239,7 +243,7 @@ function Example() {
           </p>
         </div>
       </Card>
-      <Card onClick={() => setSelected(!selected)} selected={selected}>
+      <Card selected={selected}>
         <img
           className="h-128 w-full object-cover"
           src="https://source.unsplash.com/random/404x404"
@@ -248,8 +252,10 @@ function Example() {
         <div className="p-16">
           <p className="text-12 text-gray-300">DNB Eiendom</p>
           <p>
-            Stilfull og gjennomgående 3-roms m/balkong. Oppusset i 2019. Inkl.
-            bl.a. vv/fyring.
+            <Clickable checkbox onClick={() => setSelected(!selected)}>
+              Stilfull og gjennomgående 3-roms m/balkong. Oppusset i 2019. Inkl.
+              bl.a. vv/fyring.
+            </Clickable>
           </p>
           <p className="text-14 text-gray-400 mb-4">Bøgata 25C, 0655 Oslo</p>
           <p className="font-bold my-8">
@@ -360,35 +366,39 @@ function Example() {
 
   return (
     <div>
-      <Card
-        selected={checked}
-        onClick={() => setChecked(!checked)}
-        className="mt-32 w-max"
-      >
-        <div className="p-24 flex items-center">
-          <DeadToggle checkbox checked={checked} className="-mt-8" />
-          <Clickable checkbox labelClassName="ml-12">
-            Checkbox in a card
-          </Clickable>
-        </div>
+      <Card selected={checked} className="mt-32 w-max p-24 flex items-center">
+        <DeadToggle checkbox checked={checked} className="-mt-8" />
+        <Clickable
+          checkbox
+          labelClassName="ml-12"
+          onClick={() => setChecked(!checked)}
+        >
+          Checkbox in a card
+        </Clickable>
       </Card>
 
       <div className="flex gap-32 mt-32">
-        <Card selected={selected === 'a'} onClick={() => setSelected('a')}>
-          <div className="p-24 flex items-center">
-            <DeadToggle radio checked={selected === 'a'} className="-mt-8" />
-            <Clickable radio labelClassName="ml-12">
-              Radio in a card - A
-            </Clickable>
-          </div>
+        <Card selected={selected === 'a'} className="p-24 flex items-center">
+          <DeadToggle radio checked={selected === 'a'} className="-mt-8" />
+          <Clickable
+            radio
+            labelClassName="ml-12"
+            name="gfdhjk2"
+            onClick={() => setSelected('a')}
+          >
+            Radio in a card - A
+          </Clickable>
         </Card>
-        <Card selected={selected === 'b'} onClick={() => setSelected('b')}>
-          <div className="p-24 flex items-center">
-            <DeadToggle radio checked={selected === 'b'} className="-mt-8" />
-            <Clickable radio labelClassName="ml-12">
-              Radio in a card - B
-            </Clickable>
-          </div>
+        <Card selected={selected === 'b'} className="p-24 flex items-center">
+          <DeadToggle radio checked={selected === 'b'} className="-mt-8" />
+          <Clickable
+            radio
+            labelClassName="ml-12"
+            name="gfdhjk2"
+            onClick={() => setSelected('b')}
+          >
+            Radio in a card - B
+          </Clickable>
         </Card>
       </div>
     </div>
@@ -415,34 +425,32 @@ function Example() {
     <div className="flex">
       <Card
         flat
-        className="py-12 px-16 w-max"
+        className="py-12 px-16 w-max flex items-center"
         selected={selected === 'a'}
-        onClick={() => setSelected('a')}
       >
-        <div className="flex items-center">
-          <DeadToggle radio checked={selected === 'a'} className="-mt-6" />
-          <div className="ml-16">
-            <h4 className="mb-0">
-              <Clickable radio>Purchase foo</Clickable>
-            </h4>
-            <p className="mb-0 text-14">520 kr/mnd</p>
-          </div>
+        <DeadToggle radio checked={selected === 'a'} className="-mt-6" />
+        <div className="ml-16">
+          <h4 className="mb-0">
+            <Clickable radio name="purchase" onClick={() => setSelected('a')}>
+              Purchase foo
+            </Clickable>
+          </h4>
+          <p className="mb-0 text-14">520 kr/mnd</p>
         </div>
       </Card>
       <Card
         flat
-        className="py-12 px-16 w-max ml-20"
+        className="py-12 px-16 w-max ml-20 flex items-center"
         selected={selected === 'b'}
-        onClick={() => setSelected('b')}
       >
-        <div className="flex items-center">
-          <DeadToggle radio checked={selected === 'b'} className="-mt-6" />
-          <div className="ml-16">
-            <h4 className="mb-0">
-              <Clickable radio>Purchase bar</Clickable>
-            </h4>
-            <p className="mb-0 text-14">124 kr/mnd</p>
-          </div>
+        <DeadToggle radio checked={selected === 'b'} className="-mt-6" />
+        <div className="ml-16">
+          <h4 className="mb-0">
+            <Clickable radio name="purchase" onClick={() => setSelected('b')}>
+              Purchase bar
+            </Clickable>
+          </h4>
+          <p className="mb-0 text-14">124 kr/mnd</p>
         </div>
       </Card>
     </div>

diff --git a/packages/card/src/component.tsx b/packages/card/src/component.tsx
@@ -2,9 +2,17 @@ import React from 'react';
 import { card as c } from '@fabric-ds/css/component-classes';
 import { classNames } from '@chbphone55/classnames';
 import { CardProps } from './props';
+import { useLogDeprecationWarning } from '../../utils/src';
 
 export function Card(props: CardProps) {
   const { as = 'div', children, flat, ...rest } = props;
+
+  useLogDeprecationWarning({
+    condition: !!props.onClick,
+    message:
+      "'onClick' prop in Card is deprecated. Use Clickable component to handle click events in Cards.",
+  });
+
   return React.createElement(
     as,
     {
@@ -17,6 +25,7 @@ export function Card(props: CardProps) {
         [props.selected ? c.cardFlatSelected : c.cardFlatUnselected]:
           props.flat,
       }),
+      // @balbinak(08.11.22): onClick support in Card is deprecated. Remove when Fabric React users are ready for this major change
       tabIndex: props.onClick ? 0 : undefined,
       onKeyDown: props.onClick
         ? (e) => {
@@ -43,18 +52,16 @@ export function Card(props: CardProps) {
         </button>
       )}
 
-      {!props.onClick && props.selected && (
-        <span role="checkbox" aria-checked="true" aria-disabled="true" />
+      {!props.flat && (
+        <div
+          className={classNames({
+            [c.cardOutline]: true,
+            [props.selected ? c.cardOutlineSelected : c.cardOutlineUnselected]:
+              true,
+          })}
+        />
       )}
 
-      <div
-        className={classNames({
-          [c.cardOutline]: !props.flat,
-          [props.selected ? c.cardOutlineSelected : c.cardOutlineUnselected]:
-            !props.flat,
-        })}
-      />
-
       {children}
     </>,
   );

diff --git a/packages/card/src/props.tsx b/packages/card/src/props.tsx
@@ -26,7 +26,7 @@ export interface CardProps {
   className?: string;
 
   /**
-   * When the card is clicked
+   * When the card is clicked (deprecated)
    */
   onClick?: () => void;
 }