AitorDB / home-assistant-sun-card

Home assistant sun card based on Google weather design
MIT License
407 stars 184 forks source link

Restructuring to acommodate new features #64

Closed bjornomy closed 3 years ago

bjornomy commented 3 years ago

This PR contains a major rewrite/restructure of this component. This is by no means trying to undermine the great work of the creator.

The goal was to make it easier to develop new features and accommodate for more configuration, as well as introducing some opinionated best practices.

An (non-exhausted) list of changes/additions:

AitorDB commented 3 years ago

Awesome! The list of changes/additions looks really good. It may take me a few days until I can properly take a look to this because of lack of time, but looks promising. Thanks!

bjornomy commented 3 years ago

Thanks for good input! I don't particularly mind the wait, as long as we don't merge too much stuff into develop, which might cause conflicts. I'll rebase when we have resolved all conversations.

bjornomy commented 3 years ago

The branch is now rebased on the latest develop

AitorDB commented 3 years ago

I just built it and ran some tests on Home Assistant, it needs a small fixes on Header and Footer because this.times and this.data can be undefined sometimes. An easy fix would be to just change this.fields?.dawn to this.fields?.dawn && this.times?.dawn and similar with the rest of elements, but I'll leave that to your choise. Also we will have to tweak styles a little bit, but happy to do that myself if you don't want to or as part of other PR before releasing.

PC size

Phone size

bjornomy commented 3 years ago

I just built it and ran some tests on Home Assistant, it needs a small fixes on Header and Footer because this.times and this.data can be undefined sometimes. An easy fix would be to just change this.fields?.dawn to this.fields?.dawn && this.times?.dawn and similar with the rest of elements, but I'll leave that to your choise. Also we will have to tweak styles a little bit, but happy to do that myself if you don't want to or as part of other PR before releasing.

PC size

Phone size

Tried to fix this in the helper function. Please tell me if I misunderstood.

As for styles, I think it would be best to do it in a separate issue, as this one is a bit large already 😅

AitorDB commented 3 years ago

I just built it and ran some tests on Home Assistant, it needs a small fixes on Header and Footer because this.times and this.data can be undefined sometimes. An easy fix would be to just change this.fields?.dawn to this.fields?.dawn && this.times?.dawn and similar with the rest of elements, but I'll leave that to your choise. Also we will have to tweak styles a little bit, but happy to do that myself if you don't want to or as part of other PR before releasing. PC size Phone size

Tried to fix this in the helper function. Please tell me if I misunderstood.

As for styles, I think it would be best to do it in a separate issue, as this one is a bit large already

Yes, sorry, I didn't add enough information, the issue is for example on SunCardHeader.ts:L36, the code try to access this.time.sunrise, however this.times sometimes can be undefined, which throws an error Cannot read property 'sunrise' of undefined.

Happy to keep styles for another moment

bjornomy commented 3 years ago

Ahh, that was an oversight on my part. Should be fixed now.