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

calendar data cleanup #62

Merged
merged 2 commits into from
Dec 16, 2024

Conversation

mgrfilipmarek
Copy link
Collaborator

No description provided.

@mgrfilipmarek mgrfilipmarek requested a review from rine77 December 13, 2024 10:24
@mgrfilipmarek mgrfilipmarek linked an issue Dec 13, 2024 that may be closed by this pull request
Copy link
Owner

@rine77 rine77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for contribution!
Tho I dont really get why we remove room here...?

Copy link
Owner

@rine77 rine77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for contribution!
Tho I dont really get why we remove room here...?

@mgrfilipmarek
Copy link
Collaborator Author

Room is stored in property location, so I don`t see a reason to have it at multiple places.

Thanks for contribution! Tho I dont really get why we remove room here...?

@rine77
Copy link
Owner

rine77 commented Dec 15, 2024

Room is stored in property location, so I don`t see a reason to have it at multiple places.

Thanks for contribution! Tho I dont really get why we remove room here...?

We dont have it in calendar anymore with your change... but: we want the room to be presented in calendar event

@mgrfilipmarek
Copy link
Collaborator Author

mgrfilipmarek commented Dec 16, 2024

Im not sure what you mean that we will not have it anymore. As you can see at the line 135 in the code, there is location=room. And according documentation, there is location attribute in event: https://developers.home-assistant.io/docs/core/entity/calendar/#calendarevent
Before my change, we had room information at two places - in location and also in description. I think, location is correct place to have it.
Screen from current impl with duplicit info (ha developers tools):
image

@rine77
Copy link
Owner

rine77 commented Dec 16, 2024

Im not sure what you mean that we will not have it anymore.

In calendar we have no room anymore with your change (pic1) and without (pic2)

image
image

@mgrfilipmarek
Copy link
Collaborator Author

Ah, ok, now I understand where is the problem.
In standard HA calendar implementation, location is not displayed (this can be considered as bug in ha impl. - https://community.home-assistant.io/t/show-attribute-location-in-calendar-events/628180).

As Im atomic-calendar-revive card, I did not really noticed that its missing in basic calendar.
Btw. I created also WTH post: https://community.home-assistant.io/t/wth-why-calendar-location-property-is-not-displayed/811756.

So I will change the description in feature request a bit and also adjust PR.

@rine77
Copy link
Owner

rine77 commented Dec 16, 2024 via email

Copy link
Owner

@rine77 rine77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, thanks!
So we dont have "Unknown" room anymore but no room entry at all.

@rine77 rine77 merged commit 7639ff6 into main Dec 16, 2024
4 checks passed
@rine77 rine77 deleted the 61-feature_request-cleanup-data-in-calendar-event branch December 16, 2024 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feature_request] cleanup data in calendar event
2 participants