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

Cover position tile feature #16110

Merged
merged 5 commits into from
Aug 18, 2023

Conversation

Misiu
Copy link
Contributor

@Misiu Misiu commented Apr 7, 2023

Breaking change

Proposed change

Add cover position tile feature.

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:

Misiu added 2 commits April 7, 2023 22:27
rollback changes done by VS Code
@TillFleisch
Copy link
Contributor

I like the idea. For consistency this feature should also be available for tilt positions.
Ideally the slider would look similar to the one in this dialog:
image

@Misiu
Copy link
Contributor Author

Misiu commented Apr 10, 2023

I like the idea. For consistency this feature should also be available for tilt positions.
Ideally the slider would look similar to the one in this dialog:

sure, but in a future PR :) let's add features one by one.

@@ -38,6 +39,7 @@ type SupportsFeature = (stateObj: HassEntity) => boolean;

const FEATURE_TYPES: FeatureType[] = [
"cover-open-close",
"cover-position",
Copy link
Contributor

Choose a reason for hiding this comment

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

The naming convention for cover-features needs a rework if we introduce tilt-sliders.
Maybe the position feature should be called "cover-position-slider"?

.value=${value}
min="0"
max="100"
.step="1"
Copy link
Contributor

Choose a reason for hiding this comment

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

The default value of step in ha-control-slider.ts is 1. Why should we be explicit here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was wondering if the cover has the option to set the step to a different value, but I didn't find this.
So if there is no such option we can remove this line.

Copy link
Contributor

Choose a reason for hiding this comment

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

I assume you are using the Demo Integration in the development environment to test this feature.
The demo covers provided by the Demo integration cannot be set to single percent values.
Here you can see that the target position (same goes for tilt) is rounded in 10 percent steps.

If you go from 20% to 21% percent the demo cover will close/open fully as a result of rounding the value (this behavior could be considered a bug).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TillFleisch I had a hard time trying to use my HA instance with the frontend during the development, so yes, I set up a new instance in dev containers and used the demo integration.

But the idea behind the step was to allow the user to customize the step size, but let's remove that for now.

@TillFleisch
Copy link
Contributor

TillFleisch commented Apr 10, 2023

sure, but in a future PR :) let's add features one by one.

If we change the PR title to "Cover tile slider features", I think it's fine.
Someone with more experience in this repo (i.e., a maintainer ) should make this decision.

@Misiu
Copy link
Contributor Author

Misiu commented Apr 19, 2023

@piitaya as you are the author of tiles and features, can I kindly ask you for a review? I want to get this into the next release.
If approved I can work on the tilt slider (as requested by TillFleisch)

@piitaya
Copy link
Member

piitaya commented Apr 19, 2023

The cover position slider is inverted in the more-info dialog (to looks like a real shutter or garage) because cover are mostly closed when position is at the bottom and open when position is at the top. I don't know how we can reflect this behavior in the horizontal tile.

With your proposal, the cover is open when slider is fully colored. That's not the case in the more info dialog.

@Misiu
Copy link
Contributor Author

Misiu commented Apr 19, 2023

@piitaya thank you for the fast feedback.
I can change the slider behaviour, but I agree that the slider in more info is more intuitive.
My inspiration was the mushroom cover card:
image

@piitaya
Copy link
Member

piitaya commented Aug 18, 2023

We discussed about the slider behavior with HA frontend team and we decided to invert the slider to have a more logical UX and consistency with more info.

CleanShot 2023-08-18 at 10 30 09
CleanShot 2023-08-18 at 10 29 52

@piitaya piitaya enabled auto-merge (squash) August 18, 2023 08:34
@Misiu
Copy link
Contributor Author

Misiu commented Aug 18, 2023

@piitaya if I understand correctly, the user will be able to select if he wants to have the slider inverted, so the user will decide if the position on the left means the cover is closed or opened.
What about tilt? I think this should be a separate feature in a separate PR.

@piitaya
Copy link
Member

piitaya commented Aug 18, 2023

The slider will represent the shutter and not the opening percentage like we have in the more info
So the behavior will be inverted compared to Mushroom. I will look to add an option to mushroom cover card to have the same behavior.

@piitaya
Copy link
Member

piitaya commented Aug 18, 2023

@Misiu And yes, tilt cover position should be a separate feature. You can open it if you want otherwise I will do it 🙂

@piitaya piitaya merged commit 705b6ae into home-assistant:dev Aug 18, 2023
@Misiu
Copy link
Contributor Author

Misiu commented Aug 18, 2023

@Misiu And yes, tilt cover position should be a separate feature. You can open it if you want otherwise I will do it 🙂

I don't have that much free time lately so I'm not sure when I'll be able to create that feature, so this might not get added in 2023.9, but I'm sure you'll add it in no time :)

About the position control, I disagree, that the close should be on the right.
In the case of Shelly cover with position 0% means the cover is fully closed, and 100% means the cover is fully open.
In all software, I used and developed, the sliders always had consistent behavior - on the left is a smaller value, and on the right, it's bigger, so the user always has a consistent experience (in LTR direction)
Ref: https://m3.material.io/components/sliders/specs#07c8b66b-2122-4a22-8e9f-21da97ae8a2e or just look on Google for horizontal slider ux.

Maybe an option to invert the logic? But isn't that added in 8f62671?

@piitaya
Copy link
Member

piitaya commented Aug 18, 2023

I see your point. For Home assistant, position 0% = closed and position 100% open like any other system.
In this PR, 100% is on the left and 0% is on the right. Maybe we can rename "current position" with "closing percentage" so we can have 0% on the left and 100% on the right. Having the active part on the right feels very strange for LTR people 😅

@Misiu
Copy link
Contributor Author

Misiu commented Aug 18, 2023

Not sure what would be the best solution.
The "inverted" option could invert the displayed values and control, This will let the user to decide.
We can also rename the feature, so we get the 0% on the left behavior.

@piitaya
Copy link
Member

piitaya commented Aug 18, 2023

I don't think any user wants that 😅

CleanShot 2023-08-18 at 11 31 38
CleanShot 2023-08-18 at 11 31 25

And by using the opening percentage as active zone, it will be confusing because the active zone will change between the card and more info)
CleanShot 2023-08-18 at 11 39 35
CleanShot 2023-08-18 at 11 39 21

@Misiu Misiu deleted the cover-position-tile-feature branch August 24, 2023 05:55
@Misiu
Copy link
Contributor Author

Misiu commented Aug 24, 2023

@piitaya I've created #17687.
maybe not ideal, but solves the confusion (at least for me 🙂)

@piitaya
Copy link
Member

piitaya commented Aug 24, 2023

I don't understand why it solves confusion. In one case, the slider will be inverted with more info.

@Misiu
Copy link
Contributor Author

Misiu commented Aug 24, 2023

I agree that it will be reverted, but in more info we have this view:
image
so visually you see what is the cover position and when it will move (you move the handle down, the cover moves down)
The handle on the slider represents the edge of the cover.

With my proposed configuration option, we can change the slider direction, so it aligns with, for example light brightness tile feature:
image

for brightness, 0% is on the left, same with the cover (with my change), if the user adds multiple tiles this will give them a consistent feel.

I can rename the option, so the user won't see Inverted direction (this might be confusing), but he will be able to pick from two options:
control close position
control open position

What do you think?

@piitaya
Copy link
Member

piitaya commented Aug 24, 2023

The active zone should be the same for more-info and tile feature because visually it's the same element : a slider with handle. If we choose to color the closed part on more info, it must be the same on tile card.
Adding option just lead to confusion don't you think?

@Misiu
Copy link
Contributor Author

Misiu commented Aug 24, 2023

That's a valid argument 🙂
I personally don't mind different behavior in more info and in the tile feature. For me, consistent look and experience is where 0% is on the left. so with multiple tiles, I'll always have 0% on the left. I don't use more info that much for covers.

Adding a way to pick a direction is an option (the default behavior is the one that is currently in the dev branch), so the user will be able to choose if he wants the different behavior or not.

Maybe I'll create a discussion and we'll see what others have to say, hopefully with more attention and user opinions we'll work out something or even convince the UI/UX team to add an option to pick the direction.

I can always try to create a custom tile feature and add it as an external JS file.
Not sure if the editor and translations will work with custom feature and I couldn't find any example in typescript to use as a template.

@Misiu
Copy link
Contributor Author

Misiu commented Aug 24, 2023

@piitaya another proposal that hopefully will change your mind 🙂
image
The active zone is the same as in more info and we can choose if we want to close on the left or on the right.

I've added:

.mode=${this._config.inverted_direction ? "end" : "start"}

@michelotten
Copy link

That's a valid argument 🙂 I personally don't mind different behavior in more info and in the tile feature. For me, consistent look and experience is where 0% is on the left. so with multiple tiles, I'll always have 0% on the left. I don't use more info that much for covers.

Adding a way to pick a direction is an option (the default behavior is the one that is currently in the dev branch), so the user will be able to choose if he wants the different behavior or not.

Maybe I'll create a discussion and we'll see what others have to say, hopefully with more attention and user opinions we'll work out something or even convince the UI/UX team to add an option to pick the direction.

I can always try to create a custom tile feature and add it as an external JS file. Not sure if the editor and translations will work with custom feature and I couldn't find any example in typescript to use as a template.

I have awning mounted to the outside of the house. When it is fully closed, the tile card slider is fully to the right and more info dialog fully to the bottom. In both screens slider is fully colored. For me it makes more sense to align this with brightness sliders:
Awning closed? Slider at bottom / left + NOT colored
Awning fully opened? Slider at top / right + fully colored.

@github-actions github-actions bot locked and limited conversation to collaborators Sep 5, 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