Esri / calcite-design-system

A monorepo containing the packages for Esri's Calcite Design System
https://developers.arcgis.com/calcite-design-system/
Other
289 stars 76 forks source link

alert: auto-close-duration required for custom-elements but not dist #6363

Closed mpayson closed 1 year ago

mpayson commented 1 year ago

Actual Behavior

When using the dist build, an auto-closing calcite-alert will work when just the auto-close attribute is set.

When using the custom elements build, calcite-alert needs both theauto-close and auto-close-duration attributes to be set, otherwise the alert will immediately dismiss and not display to the user.

Expected Behavior

auto-close-duration is not required (or is required) for both distributions and this is documented

Reproduction Sample

~https://codepen.io/mpayson/pen/zYLaELw~ https://codesandbox.io/s/cc-bug-calcitealert-autoclose-autocloseduration-9ht5xd?file=/src/App.js

Reproduction Steps

The codepen uses the dist build, and it looks like the auto-close-duration property is automatically set on the element, despite it not being set in the markup

  1. Open the sample
  2. Observe alert doesn't open on load
  3. Add autoCloseDuration={"medium"} to alert
  4. Observe alert now loads with autoClose
1__215131605-4b33d2c2-9081-4b08-b89f-3d37f5b64162

-27 at 10 58 23 AM

In a project that uses the custom-elements build, I used the same markup with different results

return (
    <calcite-alert
      icon="rangefinder"
      kind="brand"
      open
      label="A report alert"
      auto-close
    >
      <div slot="title">Trail Camera Report </div>
      <div slot="message">We thought you might want to take a look</div>
      <calcite-link slot="link">Take action</calcite-link>
    </calcite-alert>
  );
2__215131567-292f55c9-7741-43f1-8796-23258cbeb09c

Reproduction Version

1.0.3 (repro is in 1.2.0)

Relevant Info

custom-elements build

Regression?

No response

Impact

Seems low with the workaround, but should be mitigated as its not a great bug.

Esri team

ArcGIS Online

macandcheese commented 1 year ago

Tangential to the issue encountered here, but this prompted the thought - should we have a single, recommended duration length (current “medium” or otherwise)?

Cc @SkyeSeitz @ashetland

ashetland commented 1 year ago

Tangential to the issue encountered here, but this prompted the thought - should we have a single, recommended duration length (current “medium” or otherwise)?

Cc @SkyeSeitz @ashetland

I like this idea for consistency. The current short duration is very fast.

jcfranco commented 1 year ago

@mpayson Could you share a repro case or snippet using the custom-elements build?

geospatialem commented 1 year ago

Added additional context from #6824, there's also a workaround: https://codesandbox.io/s/calcitealert-autoclose-forked-r2zc46?file=/src/App.js

geospatialem commented 1 year ago

Research will be conducted in the upcoming June milestone for next steps.

driskull commented 1 year ago

I don't think we should be doing the following:

@Prop({ reflect: true }) autoCloseDuration: AlertDuration = this.autoClose ? "medium" : null;

If autoClose is initially false then this default autoCloseDuration won't be set. We should probably prohibit this pattern.

I'll open a PR to remove this logic and just have it set to "medium" by default.

github-actions[bot] commented 1 year ago

Installed and assigned for verification.

geospatialem commented 1 year ago

Verified in 1.5.0-next.5.