Esri / calcite-design-system

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

[Action-Pad] potentially unused mutation observer #10353

Closed maxpatiiuk closed 1 month ago

maxpatiiuk commented 2 months ago

Check existing issues

Actual Behavior

Action-pad component creates a mutation observer:

https://github.com/Esri/calcite-design-system/blob/6f42a868f5cc65ae8a7c89f22ad9057b90fe4bca/packages/calcite-components/src/components/action-pad/action-pad.tsx#L152C3-L154

However once created, that observer is not used - observer() or unobserve() is never called on it.

If I am reading the code right, this observer has no effect.

Expected Behavior

Either observer is supposed to be used, and it's a bug that it's not used, or it should be removed as dead code.

For an example of mutation observer being used, see how it's used in Action:

Reproduction Sample

-

Reproduction Steps

-

Reproduction Version

v2.12.2

Relevant Info

No response

Regression?

No response

Priority impact

impact - p3 - not time sensitive

Impact

No response

Calcite package

Esri team

ArcGIS Maps SDK for JavaScript

driskull commented 2 months ago

Seems like it should be used in order to update newly slotted calcite-action-group elements with the layout of them.

maxpatiiuk commented 2 months ago

Looks like popover has the same issue:

https://github.com/Esri/calcite-design-system/blob/4160af11ccb6bfb79314720e6f53b811f1cc6cb4/packages/calcite-components/src/components/popover/popover.tsx#L252

driskull commented 2 months ago

I think we need an eslint rule to make sure mutation observers are observed and disconnected within a file :) cc @jcfranco

maxpatiiuk commented 2 months ago

I think we need an eslint rule to make sure mutation observers are observed and disconnected within a file :) cc @jcfranco

It might be better to just create a controller for that - the controller will take care of observing connected/disconnect.

Controllers would simplify a lot of boilerplate you are having - connectMessages, connectLocalized, setComponentLoaded, etc - all of that will go away

github-actions[bot] commented 1 month ago

Installed and assigned for verification.

DitwanP commented 1 month ago

🍠 Verified locally on dev