vmware-clarity / core

Clarity is a scalable, accessible, customizable, open-source design system built with web components. Works with any JavaScript framework, created for enterprises, and designed to be inclusive.
https://clarity.design
MIT License
163 stars 42 forks source link

CdsAlert retains incorrect status upon re-rendering #189

Open astorije-vmware opened 1 year ago

astorije-vmware commented 1 year ago

Describe the bug

When conditionally rendering alerts, if one of them has the status set for CdsAlert and another alert (without status) gets rendered in its place, the first status is incorrectly kept. This is better demonstrated in the sandbox below.

How to reproduce

Steps to reproduce the behavior:

  1. Go to https://codesandbox.io/s/cds-alert-sticky-status-obs0dg
  2. Observe the info alert group with a loading alert
  3. Click on the button to toggle alert group status
  4. Even though the danger alert does not have a status, it keeps the status from the previous alert

Expected behavior

The danger alert should have no progress circle indicator.

Versions

Clarity project:

Clarity version:

Framework:

Framework version:

React 18

ashleyryan commented 1 year ago

Yeah so React is attempting to re-use the alert group instead of unmounting and mounting a new one. We have logic that runs the sync state when the slot changes - so when alerts are added or removed, but unfortunately not when properties on alerts change, which is what react is doing. I tried adding a listener for prop changes on the alerts, but that caused an infinite loop.

You can work around this by setting a key on the alerts or on the alert groups, that will tell react to destroy the element and create a new one:

https://codesandbox.io/s/cds-alert-sticky-status-forked-e376zl?file=/src/App.tsx

astorije-vmware commented 1 year ago

Yeah, we're using that workaround in a few places IIRC, but the current behavior makes things easily subject to errors that are tough to detect (especially because they can be unpredictable based on the rest of the component tree). Our React developers will definitely run into this and might not always notice it :/