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

Add label to location selector #17402

Merged
merged 3 commits into from
Sep 25, 2023
Merged

Conversation

joostlek
Copy link
Member

Proposed change

Before this change, the label of a location selector wasn't used even tho an integration might have given a label to support the type of data going in.
Before:
image
After:
image

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:

@@ -26,6 +26,7 @@ export class HaLocationSelector extends LitElement {

protected render() {
return html`
${this.label ? this.label : ""}
Copy link
Member

Choose a reason for hiding this comment

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

I think it was omitted on purpose, as this is kinda assumed at this point?

@matthiasdebaat WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree. Why do you want to add this @joostlek?

Copy link
Member Author

Choose a reason for hiding this comment

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

I found it strange that we do define this label in core, but it isn't shown. For example at Tankerkoenig, you just get a map without any context
image
(of course this would be solved with a description or something).

And maybe for integrations that require 2 locations (google travel time might be an example) it would be useful to add some context.

But personally I just noticed that this wasn't consistent so I am also fine if we just keep it as is.

Copy link
Member

Choose a reason for hiding this comment

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

I think it makes sense to add this, the user of the form should make the decision if the label is needed or not, for example if there are multiple locations to be picked

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fair point. It's strange that the name doesn't show up when you set one. Though in the example of Tankerkoenig, the name is required and not a decision of the user.

@frenck frenck requested a review from matthiasdebaat July 24, 2023 17:30
@matthiasdebaat
Copy link
Collaborator

image

Let's give it a bit more spacing between the name and map and use the same header as we use for History and Logbook.
CleanShot 2023-08-02 at 10 34 53@2x

@joostlek
Copy link
Member Author

joostlek commented Aug 2, 2023

image

@spacegaier
Copy link
Member

I am going to close and re-open since that should restart the stuck CI checks

@spacegaier spacegaier closed this Sep 7, 2023
@spacegaier spacegaier reopened this Sep 7, 2023
@spacegaier
Copy link
Member

Styling wise I am wondering if we shouldn't follow the style of "Fuel types" since the map is basically just a form input and not a separate section like logbook or history? Space between label and map looks good to me.

@joostlek
Copy link
Member Author

joostlek commented Sep 7, 2023

I was thinking the same, but I am definitely not an UIUX expert

@matthiasdebaat
Copy link
Collaborator

matthiasdebaat commented Sep 21, 2023

Styling wise I am wondering if we shouldn't follow the style of "Fuel types" since the map is basically just a form input

I agree. Had a bad assumption that it was the same font as logbook.

@bramkragten bramkragten enabled auto-merge (squash) September 25, 2023 13:28
@bramkragten bramkragten merged commit 6b33b4e into home-assistant:dev Sep 25, 2023
@joostlek joostlek deleted the location_label branch September 25, 2023 13:46
KTibow pushed a commit to KTibow/frontend-1 that referenced this pull request Sep 27, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Sep 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants