Esri / calcite-design-system

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

bug(calcite-button): renders incorrectly if an HTML comment is "slotted in" #10076

Open maxpatiiuk opened 3 months ago

maxpatiiuk commented 3 months ago

Check existing issues

Actual Behavior

<calcite-button> thinks that it has slotted in content if you provide an HTML comment as a child to it

https://github.com/Esri/calcite-design-system/blob/5478c2fcb67dc96cb98788419068eec47944ad1b/packages/calcite-components/src/components/button/button.tsx#L351-L357

Expected Behavior

<calcite-button> should not consider comment to be a slotted in content.

Reproduction Sample

https://codepen.io/maxpatiiuk/pen/mdZpWJb?editors=1000

Reproduction Steps

  1. Open codepen
  2. See that presence of a comment causes element to render incorrectly

Reproduction Version

2.11.1

Relevant Info

No response

Regression?

No response

Priority impact

impact - p2 - want for an upcoming milestone

Impact

When an element is rendered using lit-html templates, comments may be inserted by lit-html - we don't have much control over that. Those comments are causing calcite to not render correctly.

Our exact code in question (from the arcgis-home component in @arcgis/map-components):

        <calcite-button
          class={globalCss.widgetButton}
          disabled={state === "disabled"}
          iconStart={isLoading ? undefined : (icon ?? undefined)}
          kind="neutral"
          label={messages.componentLabel}
          onClick={this._go}
          // Workaround for https://github.com/Esri/calcite-design-system/issues/8490
          scale={isLoading ? "s" : "m"}
          title={state === "going-home" ? messages.cancel : messages.title}
        >
          {isLoading ? (
            // Cannot use calcite-button's loading=true. See https://devtopia.esri.com/WebGIS/arcgis-js-api/pull/58358#discussion_r1106927
            <calcite-loader inline label="" />
          ) : null}
        </calcite-button>

Calcite package

Esri team

ArcGIS Maps SDK for JavaScript

maxpatiiuk commented 3 months ago

I implemented a workaround, so this is no longer a must for next release, but a nice to have

driskull commented 3 months ago

I think this could be refactored to use the slotChangeHasContent dom utility function.

This issue also applies to the calcite-text-area component.