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

v3 Climate Component #86

Merged
merged 2 commits into from
May 7, 2024
Merged

v3 Climate Component #86

merged 2 commits into from
May 7, 2024

Conversation

mattsgreen
Copy link
Contributor

@mattsgreen mattsgreen commented Apr 21, 2024

v3 Climate component draft. Introduces:

  • Up/Down buttons for target temperature(s)
  • Better layout of information
  • Support for presets
  • Integration with the graph used for sensors

It could do with a bit more design work, ideally just needs a little more horizontal space

Copy link

netlify bot commented Apr 21, 2024

Deploy Preview for esphome-webserver ready!

Name Link
🔨 Latest commit 8261e56
🔍 Latest deploy log https://app.netlify.com/sites/esphome-webserver/deploys/663ab177820b7d0008d4c3fd
😎 Deploy Preview https://deploy-preview-86--esphome-webserver.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@mattsgreen mattsgreen changed the title Inital commit of Climate Component for V3 v3 Climate Component Apr 21, 2024
Copy link
Contributor

@kevireilly kevireilly left a comment

Choose a reason for hiding this comment

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

Nice work, thanks so much for the contribution. Some small not important suggestions that are non-blocking.

if (
e?.currentTarget?.domain === "sensor" ||
e?.currentTarget?.domain === "climate"
) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not important and not certain if this code is being transpiled, but if it is, a bit of redundant code can be reduced by doing something like if (!e?.currentTarget) return; as the first line within this function. That way we're not repeatedly checking and ensuring that e and currentTarget exists (if it does the first time, it will the next n times). Then also TS shouldn't require ? for each reference (e?.currentTarget?... -> e.currentTarget...).

I know this wasn't you, but a good opportunity for tiny clean up if you're willing.

let targetTemp =
target === "high"
? entity.target_temperature_high
: entity.target_temperature || entity.target_temperature_low;
Copy link
Contributor

Choose a reason for hiding this comment

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

One small improvement might be caching this choice to something like const lowTemp = entity.target_temperature || entity.target_temperature_low; and using that reference instead. Probably same for most of the Number casts used multiple times.

packages/v3/src/esp-entity-table.ts Show resolved Hide resolved
@jesserockz jesserockz marked this pull request as ready for review May 7, 2024 23:07
@jesserockz jesserockz merged commit b09657b into esphome:dev May 7, 2024
7 checks passed
RFDarter added a commit to RFDarter/esphome-webserver that referenced this pull request May 17, 2024
@RFDarter RFDarter mentioned this pull request May 17, 2024
jesserockz added a commit that referenced this pull request May 19, 2024
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.

3 participants