Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Action-Pad] potentially unused mutation observer #10353

Closed
2 of 6 tasks
maxpatiiuk opened this issue Sep 19, 2024 · 6 comments
Closed
2 of 6 tasks

[Action-Pad] potentially unused mutation observer #10353

maxpatiiuk opened this issue Sep 19, 2024 · 6 comments
Assignees
Labels
4 - verified Issues that have been released and confirmed resolved. ArcGIS Maps SDK for JavaScript Issues logged by ArcGIS SDK for JavaScript team members. bug Bug reports for broken functionality. Issues should include a reproduction of the bug. calcite-components Issues specific to the @esri/calcite-components package. estimate - 3 A day or two of work, likely requires updates to tests. impact - p3 - not time sensitive User set priority impact status of p3 - not time sensitive p - low Issue is non core or affecting less that 10% of people using the library

Comments

@maxpatiiuk
Copy link
Member

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/calcite-components
  • @esri/calcite-components-angular
  • @esri/calcite-components-react
  • @esri/calcite-design-tokens
  • @esri/eslint-plugin-calcite-components

Esri team

ArcGIS Maps SDK for JavaScript

@maxpatiiuk maxpatiiuk added bug Bug reports for broken functionality. Issues should include a reproduction of the bug. 0 - new New issues that need assignment. needs triage Planning workflow - pending design/dev review. labels Sep 19, 2024
@github-actions github-actions bot added ArcGIS Maps SDK for JavaScript Issues logged by ArcGIS SDK for JavaScript team members. calcite-components Issues specific to the @esri/calcite-components package. impact - p3 - not time sensitive User set priority impact status of p3 - not time sensitive labels Sep 19, 2024
@driskull
Copy link
Member

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

@maxpatiiuk
Copy link
Member Author

Looks like popover has the same issue:

mutationObserver: MutationObserver = createObserver("mutation", () =>

@driskull
Copy link
Member

driskull commented Sep 19, 2024

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

@geospatialem geospatialem added p - low Issue is non core or affecting less that 10% of people using the library 2 - in development Issues that are actively being worked on. estimate - 3 A day or two of work, likely requires updates to tests. and removed 0 - new New issues that need assignment. needs triage Planning workflow - pending design/dev review. labels Sep 19, 2024
@maxpatiiuk
Copy link
Member Author

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

driskull added a commit that referenced this issue Sep 20, 2024
… initialization (#10355)

**Related Issue:** #10353

## Summary

- update layout on action-group elements slotted after initialization
- add test
@driskull driskull added 3 - installed Issues that have been merged to master branch and are ready for final confirmation. and removed 2 - in development Issues that are actively being worked on. labels Sep 20, 2024
Copy link
Contributor

Installed and assigned for verification.

@github-actions github-actions bot assigned geospatialem and DitwanP and unassigned driskull Sep 20, 2024
driskull added a commit that referenced this issue Sep 20, 2024
**Related Issue:** #10353

## Summary

- connect and disconnect mutation observer
@DitwanP
Copy link
Contributor

DitwanP commented Sep 20, 2024

🍠 Verified locally on dev

@DitwanP DitwanP closed this as completed Sep 20, 2024
@DitwanP DitwanP added 4 - verified Issues that have been released and confirmed resolved. and removed 3 - installed Issues that have been merged to master branch and are ready for final confirmation. labels Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - verified Issues that have been released and confirmed resolved. ArcGIS Maps SDK for JavaScript Issues logged by ArcGIS SDK for JavaScript team members. bug Bug reports for broken functionality. Issues should include a reproduction of the bug. calcite-components Issues specific to the @esri/calcite-components package. estimate - 3 A day or two of work, likely requires updates to tests. impact - p3 - not time sensitive User set priority impact status of p3 - not time sensitive p - low Issue is non core or affecting less that 10% of people using the library
Projects
None yet
Development

No branches or pull requests

4 participants