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

Restore attributes removed from ha-entity-marker in ha-map #23603

Merged
merged 2 commits into from
Jan 7, 2025

Conversation

karwosts
Copy link
Contributor

@karwosts karwosts commented Jan 6, 2025

Proposed change

In a recent change, I inadvertently removed attributes from ha-entity-marker that several users were relying on to identify specific markers in the DOM for styling.

This change restores those attributes for maximum backward compatibility, instead of being set as properties.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue or discussion:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

@bramkragten
Copy link
Member

I don't think this is a good idea, it will impact performance just for a custom component and can be removed at any point again 🤷‍♂️

@karwosts
Copy link
Contributor Author

karwosts commented Jan 6, 2025

Ok, I did not realize there was a performance difference between setting attribute and setting property. Thanks for checking.

If there's no way forward, feel free to close this.

@Mariusthvdb
Copy link
Contributor

Mariusthvdb commented Jan 6, 2025

tbh, we never noticed any performance issue on the maps before with the attribute, even with more complex mods than the one I posted in the Discord channel.

But if that would truly be the case, and leaving out entity_id here would be permanent, would there be another possibility to get to the individual markers for the entity_id's?

If this was not planned in the first place, because of performance issues, as seems to be the case, please (re-)consider reinstating

@bramkragten
Copy link
Member

Ok, I did not realize there was a performance difference between setting attribute and setting property. Thanks for checking.

If there's no way forward, feel free to close this.

I mean, it is small, but it removes the step of converting the attribute to a property.

bramkragten
bramkragten previously approved these changes Jan 6, 2025
@bramkragten bramkragten added this to the 2025.1 milestone Jan 6, 2025
@karwosts
Copy link
Contributor Author

karwosts commented Jan 6, 2025

Should I have just done only entity-id as an attribute and left the other 3 as properties?

@bramkragten
Copy link
Member

bramkragten commented Jan 6, 2025

Should I have just done only entity-id as an attribute and left the other 3 as properties?

If thats the only one thats needed, yes 👍

Actually, I think I would prefer to fix this inside ha-entity-marker and set reflect: true on entityId, then it is fixed in 1 place and it doesnt matter how it is set, by property or attribute.

@MindFreeze MindFreeze merged commit 6ef799b into home-assistant:dev Jan 7, 2025
15 checks passed
@karwosts karwosts deleted the map-marker-attributes branch January 7, 2025 14:20
bramkragten pushed a commit that referenced this pull request Jan 9, 2025
* Restore attributes removed from ha-entity-marker in ha-map

* Use Reflect
@Mariusthvdb
Copy link
Contributor

letting you know this is working again, thank yvm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants