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

Convert thermostat to round-slider #3734

Merged
merged 17 commits into from
Oct 17, 2019

Conversation

iantrich
Copy link
Member

@iantrich iantrich commented Sep 17, 2019

Closes #3622
Closes #2756
Closes #3711
Closes #3889
Closes #3891

@iantrich
Copy link
Member Author

iantrich commented Sep 17, 2019

  • Scaling
  • Handles are not properly themed yet. Edit: I actually kind of like the colored handles rather than the white. It looks like the new slider does not have a border var on the handles yet, is that right @thomasloven?

@iantrich iantrich changed the title WIP: Convert to round-slider WIP: Convert thermostat to round-slider Sep 17, 2019
@thomasloven
Copy link
Contributor

That's correct. I made them to look more like paper-slider than our old round sliders.

Re: card size, there should really be a minimum supported card width. 200-300 px is definitely reasonable.

@iantrich
Copy link
Member Author

@thomasloven thanks. Yeah, I like the handles as is.

@iantrich iantrich changed the title WIP: Convert thermostat to round-slider Convert thermostat to round-slider Sep 18, 2019
@iantrich
Copy link
Member Author

I think the card looks good down to ~300px
Please check the preview and try to break the theming and/or find improvements.

@thomasloven
Copy link
Contributor

ezgif com-resize (1)

I think it could do with some more work 😉

@iantrich
Copy link
Member Author

🤔 looks fine on refresh though, right?

@thomasloven
Copy link
Contributor

Only the final state. There's still the problem with the name overlapping the circle at some sizes, and the circle being taller than the card in the single column.

@iantrich
Copy link
Member Author

why did I sign up for something involving so much CSS... 🤦‍♂

@iantrich
Copy link
Member Author

@thomasloven I only get that behavior when doing what you were doing; moving the window size. On a refresh of the page it looks fine to me. Do I need to fire an iron-resize or something? This is the kind of stuff I'm quite ignorant on, still.

@thomasloven
Copy link
Contributor

I get the problem even after refresh if the window is between about 540 and 600 px wide.
image

Copy link
Member

@bramkragten bramkragten left a comment

Choose a reason for hiding this comment

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

Nice! 👍

src/panels/lovelace/cards/hui-thermostat-card.ts Outdated Show resolved Hide resolved
src/panels/lovelace/cards/hui-thermostat-card.ts Outdated Show resolved Hide resolved
src/panels/lovelace/cards/hui-thermostat-card.ts Outdated Show resolved Hide resolved
@bramkragten
Copy link
Member

We should also remove jquery and roundslider from the deps, and remove the load scripts.

@iantrich
Copy link
Member Author

We can do that in a separate PR

@iantrich
Copy link
Member Author

Last commit helps with window resizing but still struggling with scaling at certain sizes

@Misiu
Copy link
Contributor

Misiu commented Sep 19, 2019

I found a weird behavior: when You enter config mode and add this entry:

type: vertical-stack
cards:
  - cards:
      - entity: climate.upstairs
        type: thermostat
        name: Kuchnia
      - entity: climate.upstairs
        type: thermostat
        name: Sypialnia
    type: horizontal-stack
  - cards:
      - entity: climate.upstairs
        type: thermostat
        name: Pokój dzienny
      - entity: climate.upstairs
        type: thermostat
        name: Korytarz
    type: horizontal-stack

You get this view:
ha1

But when You close edit view You suddenly get this:
ha2

The easiest way to reproduce this is to add two thermostats into horizontal-stack:
ha3

@iantrich
Copy link
Member Author

I regret starting this... 😂

@Misiu
Copy link
Contributor

Misiu commented Sep 19, 2019

@iantrich even with these small issues, You version works and looks great 👏
The first problem (resizing after exiting edit mode) isn't that important - I can always hit refresh (but unfortunately not on demo site).
The only issue (for me) is placing two thermostats in horizontal-stack, but I can live with it 🙂

@iantrich
Copy link
Member Author

@Misiu it will get there, just a chore at this point 👎

@thomasloven
Copy link
Contributor

The light card doesn't scale... might be worth considering.

@iantrich
Copy link
Member Author

Now if the name is greater than 10 characters, the title size is lowered from 23px to 18px. I think that's good for now and could possible be tweaked further in another PR.

@iantrich
Copy link
Member Author

@bramkragten I think this is good to go now unless @thomasloven has anything to add. Thanks for both your help on this one.

@thomasloven
Copy link
Contributor

Now if the name is greater than 10 characters, the title size is lowered from 23px to 18px. I think that's good for now and could possible be tweaked further in another PR.

Did you perhaps forget to push that?

@iantrich
Copy link
Member Author

Now if the name is greater than 10 characters, the title size is lowered from 23px to 18px. I think that's good for now and could possible be tweaked further in another PR.

Did you perhaps forget to push that?

I sure did, thanks!

.max=${stateObj.attributes.max_temp}
@value-changing=${this._dragEvent}
@value-changed=${this._setTemperature}
></round-slider>
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I think round-slider could handle this. If you give it both low, high and value, it will show one handle if low is undefined, and two otherwise.
May be a bit confusing, though...

@iantrich
Copy link
Member Author

...can this be done...? 🤣 I never want to look at this card again

@bramkragten
Copy link
Member

...can this be done...? 🤣 I never want to look at this card again

I will assign all issues with this card to you 😉

@bramkragten bramkragten merged commit f5e3a9a into home-assistant:dev Oct 17, 2019
@Misiu
Copy link
Contributor

Misiu commented Oct 18, 2019

@iantrich congratulations 🎉

@bramkragten bramkragten mentioned this pull request Oct 23, 2019
@iantrich iantrich deleted the thermo-slider branch October 30, 2019 03:19
@github-actions github-actions bot locked and limited conversation to collaborators Jul 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants