resoai / TileBoard

A simple yet highly configurable Dashboard for HomeAssistant
MIT License
1.63k stars 278 forks source link

Localized climate options #667

Closed mikyjazz closed 3 years ago

mikyjazz commented 3 years ago
akloeckner commented 3 years ago

I'm wondering: Isn't all that logic basically an exerpt of what entityState does? https://github.com/resoai/TileBoard/blob/4126d1a0e1dee9da5477a6ee03be9fd81bdae7e1/scripts/controllers/main.js#L525

If it is, can you make use of that (even more generic) function?

E.g.:

mikyjazz commented 3 years ago

I'm wondering: Isn't all that logic basically an exerpt of what entityState does? https://github.com/resoai/TileBoard/blob/4126d1a0e1dee9da5477a6ee03be9fd81bdae7e1/scripts/controllers/main.js#L525

If it is, can you make use of that (even more generic) function?

E.g.:

  • getClimateOptions => something like mapping hvacmodes through entityState
  • getClimateCurrentOption => just map current mode through entityState
  • setClimateOption => that might be the hard case

If you look closely at the code you will see that two different properties are mapped const option = item.useHvacMode? entity.state: entity.attributes.preset_mode; I can't imagine how you would like to generalize this climate control specific thing ...

akloeckner commented 3 years ago

I can't imagine how you would like to generalize this climate control specific thing ...

Well, this part seems to be inconsistent anyways: https://github.com/resoai/TileBoard/blob/4126d1a0e1dee9da5477a6ee03be9fd81bdae7e1/scripts/controllers/main.js#L533

It uses item.states even though the state is determined by item.state. We could make it consistent by always using item.states, e.g. even if item.state is a function.

This item.states related part could be split out as, say, mapStates(). Which could in turn be re-used for the current climate option and the list of options.

For the reverse lookup, this looks like a nice solution and we already have some lodash functions in the code: https://lodash.com/docs/4.17.4#invertBy

mikyjazz commented 3 years ago

I can't imagine how you would like to generalize this climate control specific thing ...

Well, this part seems to be inconsistent anyways: https://github.com/resoai/TileBoard/blob/4126d1a0e1dee9da5477a6ee03be9fd81bdae7e1/scripts/controllers/main.js#L533

It uses item.states even though the state is determined by item.state. We could make it consistent by always using item.states, e.g. even if item.state is a function.

This item.states related part could be split out as, say, mapStates(). Which could in turn be re-used for the current climate option and the list of options.

For the reverse lookup, this looks like a nice solution and we already have some lodash functions in the code: https://lodash.com/docs/4.17.4#invertBy

item.state and item.states despite the name are two things with a completely different meaning

rchl commented 3 years ago

@mikyjazz can you update us on what is the status of this on your side?

From my side, I think some of my comments are still unresolved.

mikyjazz commented 3 years ago

hi Rafal, some changes I have implemented in his time while the ones that are missing I have not taken into consideration; if you are interested in the pull request you can make them yourself. For my part, I realized that it is not possible to carry out a version for the public and one for me only so I have decided to carry out only my fork, with the changes I like, the style aspects that I like and without respecting (or at least without asking myself many problems about it) the constraints related to coding style; in the end it will be much easier to integrate in my version what you do in the master one;) See you soon.

rchl commented 3 years ago

Merged manually to master. Thanks.