dequelabs / cauldron

https://cauldron.dequelabs.com/
Mozilla Public License 2.0
86 stars 20 forks source link

Bug: `Stepper` causes console error when using `tooltipText` attribute #1498

Open yhafez opened 3 months ago

yhafez commented 3 months ago

Steps to reproduce

Steps to reproduce

1) Create a stepper component 2) Add a tooltip and tooltipText attribute 3) Examine the devtools console

Expected Behavior

No error

Actual Behavior

An error is shown in the console, though it doesn't show up in linting or compilation and does't break the app. An example of the error can be found under "Relevant log output"

Version

6.3.0

What browsers are you experiencing the problem on?

Chrome

Relevant log output

Warning: React does not recognize the `tooltipText` prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercase `tooltiptext` instead. If you accidentally passed it from a parent component, remove it from the DOM element.
    in li (created by Step)
    in Step (created by CreateOrEditTemplate)
    in ol (created by Stepper)
    in Stepper (created by CreateOrEditTemplate)
    in form (created by CreateOrEditTemplate)
    in FormProvider (created by CreateOrEditTemplate)
    in CreateOrEditTemplate (created by IntegrationTemplates)
    in Route (created by IntegrationTemplates)
    in Switch (created by IntegrationTemplates)
    in IntegrationTemplates (created by AppRoutes)
    in Protected (created by AppRoutes)
    in SubscriptionRequired (created by AppRoutes)
    in FeatureRequired (created by AppRoutes)
    in BillingRequired (created by AppRoutes)
    in Route (created by AppRoutes)
    in Switch (created by AppRoutes)
    in AppRoutes (created by App)
    in div (created by MainContent)
    in div (created by MainContent)
    in main (created by Main)
    in Main (created by Workspace)
    in div (created by Layout)
    in Layout (created by Workspace)
    in Workspace (created by MainContent)
    in MainContent (created by MainLayout)
    in div (created by MainLayout)
    in div (created by MainLayout)
    in MainLayout (created by Layout)
    in Layout (created by App)
    in Route (created by App)
    in Switch (created by App)
    in Router (created by BrowserRouter)
    in BrowserRouter (created by App)
    in Elements (created by StripeElements)
    in StripeElements (created by App)
    in IntegrationsProvider (created by App)
    in ConfigurationProvider (created by App)
    in ProductsProvider (created by App)
    in EnterprisesProvider (created by App)
    in Provider (created by App)
    in AnalyticsProvider (created by App)
    in Provider (created by App)
    in AuthProvider (created by App)
    in Provider (created by App)
    in Maintenance (created by App)
    in GlobalToastProvider (created by App)
    in App

Anything else we should know?

Additional information

scurker commented 3 months ago

@yhafez are there any reproduction details I'm missing? There's this example on the docs site:

<Stepper>
  <Step
    status="complete"
    tooltip={<><div>Step 1: Foo</div><div>Status: Complete</div></>}
    tooltipText="Foo Complete"
  />
  <Step
    status="current"
    tooltip={<><div>Step 2: Bar</div><div>Status: Current</div></>}
    tooltipText="Bar Current"
  />
</Stepper>

But I'm not seeing any of the console output above, so I'm still trying to figure out how to reproduce.

yhafez commented 3 months ago

@scurker it looks like the issue is when the Step is rendered with this prop and tooltip is undefined. Here's my implementation:

<Stepper className={narrow ? styles.narrowStepper : undefined}>
          {steps.map(({ header }, i) => {
            const stepStatus = getStepStatus(currentStep, i);
            const stepStatusCapitalized = getCapitalizedStepStatus(
              t,
              stepStatus
            );

            return (
              <Step
                status={stepStatus}
                key={header}
                tooltip={
                  narrow ? (
                    <>
                      <Trans>
                        <div>
                          Step {{ i: i + 1 }}: {{ header }}
                        </div>
                        <div>Status: {{ stepStatusCapitalized }}</div>
                      </Trans>
                    </>
                  ) : undefined
                }
                tooltipText={
                  narrow
                    ? `${header} ${getStepStatus(currentStep, i)}`
                    : undefined
                }
              >
                {narrow ? null : `${header}`}
              </Step>
            );
          })}
        </Stepper>

narrow come from the useMediaQuery hook, but when I explicitly set it to true, the error doesn't appear. So even though tooltipText is set to undefined when tooltip is set to undefined (in the case where narrow is false), the console.error still appears in the console.

Not sure if we should explicitly handle when both are undefined in Cauldron, or if we should leave it to the user to conditionally render a separate Step component instead like so in my example:

<Stepper className={narrow ? styles.narrowStepper : undefined}>
          {steps.map(({ header }, i) => {
            const stepStatus = getStepStatus(currentStep, i);
            const stepStatusCapitalized = getCapitalizedStepStatus(
              t,
              stepStatus
            );

            return narrow ? (
              <Step
                status={stepStatus}
                key={header}
                tooltip={
                    <>
                      <Trans>
                        <div>
                          Step {{ i: i + 1 }}: {{ header }}
                        </div>
                        <div>Status: {{ stepStatusCapitalized }}</div>
                      </Trans>
                    </>
                }
                tooltipText={`${header} ${getStepStatus(currentStep, i)}`}
             / >
            ) : (
              <Step
                status={stepStatus}
                key={header}
              >
                {header}
              </Step>
            );
          })}
        </Stepper>

My inclination is that we should handle when they're both undefined so the end user doesn't need to worry about it and since the user won't know they should use this pattern instead of the pattern I was originally using. Basically just not setting the tooltipText prop when tooltip is undefined. What do you think?

scurker commented 3 months ago

I'm trying to wrap my head around on how that error is generated. We do a sanity check for the tooltip prop:

https://github.com/dequelabs/cauldron/blob/d122f709347cbf3de34d36d83e287af6e60b1d91/packages/react/src/components/Stepper/index.tsx#L21-L23

I guess we're spreading the props onto the li element, but the prop itself should be undefined. It's an unusual use case since normally the props themselves aren't conditional.