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

feat(light): add collapsible controls for light #383

Merged
merged 8 commits into from
Apr 28, 2022
Merged

Conversation

M4D1NG3R
Copy link
Contributor

@M4D1NG3R M4D1NG3R commented Apr 22, 2022

Description

implementation of the "Collapse slider when off" function from Ui Lovelace Minimalist.

Related Issue

#126

Motivation and Context

I really liked the function & implemented a workaround with the state-switch card, but thought it would be great to have this function natively.

How Has This Been Tested

I tested it with the Development server as mentioned in the README

Types of changes

  • 🐛 Bug fix (non-breaking change which fixes an issue)
  • 🚀 New feature (non-breaking change which adds functionality)
  • 🌎 Translation (addition or update a translation)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have tested the change locally.

I could need some help with translations, testing & documentation.
Also, I tried to align with the code style of this project, but I am open for critics.

@piitaya
Copy link
Owner

piitaya commented Apr 23, 2022

Hello!
I tried to implement it in the past but there were issue will the grid layout :
Screenshot_20220423-084150.jpg

The card take the full height so the option is not very useful and the UI is not consistent. I think we need to fix this issue before adding the issue.
Also, it can be an option for all cards, not just for lights. I will look how we can refactor the code to reduce the copy paste between card 🙂

Thanks I will prioritize this feature 🙂

@greghesp
Copy link
Contributor

greghesp commented Apr 23, 2022

I understand why you want to do this, but it definitely needs to be an option (I see it already is) Having the slider visible whilst the light is off, means you can tap the slider at any position which turns it on at that brightness. Without that, you can only turn it on/off

@M4D1NG3R
Copy link
Contributor Author

M4D1NG3R commented Apr 23, 2022

@piitaya thanks for the feedback, I only tested it without the grid:
image
image

Moreover, I only partially understand your problem. If I have two lights, one with and one without a slider, the card will also take the full height. So the problem exists independently of my implementation:
image

In addition, when both lights are off, the size is reduced. So the goal of saving space is fulfilled:
image

A solution to your problem would be to put every light in a vertical-stack card:

type: horizontal-stack
cards:
  - type: horizontal-stack
    cards:
      - type: vertical-stack
        cards:
          - type: custom:mushroom-light-card
            entity: light.ceiling_lights
            show_brightness_control: true
            show_color_control: true
            show_color_temp_control: true
            use_light_color: true
            enable_collapse: true
      - type: vertical-stack
        cards:
          - type: custom:mushroom-light-card
            entity: light.kitchen_lights
            show_brightness_control: true
            show_color_control: true
            show_color_temp_control: true
            use_light_color: true
            enable_collapse: true

image

@greghesp I implemented it as an option:
image

@M4D1NG3R
Copy link
Contributor Author

M4D1NG3R commented Apr 23, 2022

@piitaya
I would really like to contribute, but I am a little confused. Do I understand this correctly:
When measured by your standards, the UI in its current state is already not consistent?
Could you explain, how your preferred version would look like?

By removing height: 100%; (line 35 in src/shared/card.ts) it looks like this:
image
Is this what you want?

@M4D1NG3R
Copy link
Contributor Author

M4D1NG3R commented Apr 23, 2022

@piitaya please check again; I implemented a version which works like this:
image

Does this align with your standards?

@M4D1NG3R M4D1NG3R marked this pull request as draft April 24, 2022 17:21
@M4D1NG3R
Copy link
Contributor Author

@piitaya did you have some time to look at my implementation?
If so, I would be very happy to get some feedback from your side.

Thank you in advance!

Copy link
Owner

@piitaya piitaya left a comment

Choose a reason for hiding this comment

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

Hi 🙂 Sorry, the PR was in draft, I thought it was not ready to review 😅

Maybe a collapse_controls instead of enable_collapse can be more explicit ? What do you think?

I will investigate how we can change the layout to support filling parent height or not. I will give some tests 😅 It will be another PR.

src/cards/light-card/light-card.ts Outdated Show resolved Hide resolved
src/translations/en.json Outdated Show resolved Hide resolved
@M4D1NG3R M4D1NG3R marked this pull request as ready for review April 28, 2022 17:43
@M4D1NG3R
Copy link
Contributor Author

M4D1NG3R commented Apr 28, 2022

Hey, many thanks for the feedback!
I reverted the changes regarding the "card takes full height" issue and edited the README regarding your request.

Also, the PR is now ready for review.

@piitaya
Copy link
Owner

piitaya commented Apr 28, 2022

What about renaming enable_collapse to collapse_controls ? May be you think enable_collapse is better ? or hide_controls_when_off? 😅

@M4D1NG3R
Copy link
Contributor Author

I am open for anything which fits better in your opinion.
I will change it to collapse_controls

@piitaya piitaya changed the title feat(light): Collapse slider when off feat(light): add collapsible controls for light Apr 28, 2022
@piitaya piitaya merged commit 9cf234a into piitaya:main Apr 28, 2022
@pedolsky
Copy link

A small note: The question came up in the forum why it doesn't work.
I am not familiar with the development process here on Github. But it is confusing for users when the readme has already been updated before the corresponding release.
That aside: Great addition!

@greghesp
Copy link
Contributor

A small note: The question came up in the forum why it doesn't work. I am not familiar with the development process here on Github. But it is confusing for users when the readme has already been updated before the corresponding release. That aside: Great addition!

I think this is a HACs thing. Not overly familiar with how HACs reads things, but I believe an Info.md is needed for HACs if you want the Readme to belong only to the repo

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.

4 participants