rejuvenate / lovelace-horizon-card

Sun Card successor: Visualize the position of the Sun over the horizon.
MIT License
474 stars 35 forks source link

Fix moon phase integration #131

Open MelleD opened 7 months ago

MelleD commented 7 months ago

Pull Request Template for Home Assistant / Lovelace Card Repository

Overview

a) Fixed issue https://github.com/rejuvenate/lovelace-horizon-card/issues/122 b) Fixed or ignored broken test for language for persia fa c) Add new release pipeline

Type of Change

ThomDietrich commented 7 months ago

I'd like to fix the existing pipeline issue first before we can concentrate on your various improvements. I've created: https://github.com/rejuvenate/lovelace-horizon-card/pull/132 but that did not fix it.

MelleD commented 7 months ago

Hey @MelleD thanks for your contribution! Could you please explain your reasoning for replacing the pipeline by a different approach?

Yes off course.

The differences aren't that big. I use npm instead of yarn and you can enter a version number during the release and that's it.

The main reason is that my changes were never released and the pipeline or caching was broken somewhere. After a release it was always the old version in the release Secondly, I am not a UI expert and have seen and adapted the pipelines from our company that our experts use ;)

MelleD commented 7 months ago

I'd like to fix the existing pipeline issue first before we can concentrate on your various improvements. I've created: #132 but that did not fix it.

Ahh mhm maybe it's an encoding issue. The files are not UTF-8

ThomDietrich commented 6 months ago

Hey @MelleD, I am not in the position to review this PR. I am only one of the rejuvenate admins. I will leave it with @avataar and @edwardtfn to have a look.

In order to publish a release the pipeline is not an issue. There is an actual bug that needs fixing. Would you by any chance be able to provide a PR with the fix? https://github.com/rejuvenate/lovelace-horizon-card/actions/runs/8855612167/job/24320738547

MelleD commented 6 months ago

Hey @ThomDietrich, I don't fully understand your problems. What is unclear? Where can I help you?

But you are welcome to take my PR and changes and cherry pick whatever you want :). But at some point you should also update the dependencies for the pipeline and the JS libs. Additionally, I would advise saving all files in UTF-8.

ThomDietrich commented 6 months ago

Hey Melle, it is common practice to address one concern at a time. https://github.com/rejuvenate/lovelace-horizon-card/pull/134 by @trvrnrth resolved the bug that blocked the v1.1.0 release.

With that out of the way, I am more than happy to discuss improvements to the implemented workflow. I can see that you have added automatic dependency checks and updates, as well as a switch from yarn to npm. Furthermore you have replaced the modified suncalc3 version by @avataar (please see https://github.com/rejuvenate/lovelace-horizon-card/blob/main/suncalc3/README-HORIZON-CARD.md) by its latest upstream.

Let's take it step by step:

  1. Could you please create an independent PR to add dependabot?
  2. Are you sure the modifications in suncalc3 can be discarded? Did they find their way into main?
  3. Why is the switch from yarn to npm worthwhile?
  4. Your suggestion re UTF8 makes sense to me. Is that something you could contribute in a PR?

Thanks and best!

MelleD commented 6 months ago

@ThomDietrich how I said take what ever you want and what do you think make sense for the project as improvement. I won't have a PC at my side for the next few weeks.

  1. Dependabot you just need the yaml and enable it for the githubrepo under settings.
  2. how I said suncalc didn’t work locally so maybe you have to ignore it
  3. Our JS company developer do recommend npm with dependabot. That was the only reason and it worked straight forward also locally.
  4. Better do it with a good IDE
  5. workflow is up to you but personally I like it more to set a version by my own with some nice auto generated release notes (based on GitHub labels). And it’s also just a yaml file and the workflow yaml. Just check it out and decide.

Happy coding if something is unclear just ask. Answering with mobile phone is no problem

ThomDietrich commented 6 months ago

Okay thanks for the additional remarks! I will keep the PR open until we've accomplished the above.

I will take care of dependabot today. @tripplehelix @trvrnrth I am not a js/ts developer. Do you have any opinion or would you like to contribute on the other elements? Cheers!

trvrnrth commented 6 months ago

I'm not a js/ts dev either but here's my tuppence anyway:

  1. Dependabot is probably a nice to have.
  2. Using suncalc from upstream is nicer so long as all required local changes have been integrated. The problem experienced with the local install will have been a result of using npm rather than yarn.
  3. I do not know what the commonly held opinions on the pros and cons of yarn vs npm are.
  4. If the language files and test snapshots were not UTF-8 then that sounds likely to be the source of the problem with the persion translation.
  5. Release process is going to boil down to maintainer personal preference but very briefly skimming over what is proposed here it looks like a reasonable approach to me anyway.

Unfortunately I am realistically unlikely to have the time available to dedicate to contribute any of the changes.

cataseven commented 4 months ago

Hi I am not a developer but coding as hobby so below changes can looks stupid :) but changed this

line 2800

key: "renderMoonElement",
    value: function renderMoonElement(i18n, phase, phaseRotation) {
      if (phase === undefined) {
        return A;
      }
      var moon_phase_localized = i18n.localize("component.moon.entity.sensor.phase.state.".concat(phase.state));
      if (!moon_phase_localized) {
        moon_phase_localized = x(_templateObject2$3 || (_templateObject2$3 = _taggedTemplateLiteral(["<span title=\"Install Moon integration to get localized Moon phase name\">", " (!)</span>"])), phase.state);
      }
      return x(_templateObject3$2 || (_templateObject3$2 = _taggedTemplateLiteral(["\n      <div class=\"horizon-card-text-container\">\n        <div class=\"horizon-card-field-moon-phase\" style=\"transform: rotate(", "deg)\">\n          <ha-icon icon=\"mdi:", "\"></ha-icon>\n        </div>\n        <div class=\"horizon-card-field-value horizon-card-field-value-moon-phase\">", "</div>\n      </div>\n    "])), phaseRotation, phase.icon, moon_phase_localized);
    }

into this and now Moon Phases are rendered capitalized.

    key: "renderMoonElement",
    value: function renderMoonElement(i18n, phase, phaseRotation) {
      if (phase === undefined) {
        return A;
      }
      var moon_phase_localized = i18n.localize("component.moon.entity.sensor.phase.state.".concat(phase.state));
      if (!moon_phase_localized) {
        moon_phase_localized = x(_templateObject2$3 || (_templateObject2$3 = _taggedTemplateLiteral(["<span title=\"Install Moon integration to get localized Moon phase name\">", " (!)</span>"])), phase.state);
      }

      // Split the localized moon phase name into words, capitalize the first letter of each word, and join them back together
      moon_phase_localized = moon_phase_localized.split(' ').map(word => word.charAt(0).toUpperCase() + word.slice(1)).join(' ');

      return x(_templateObject3$2 || (_templateObject3$2 = _taggedTemplateLiteral(["\n      <div class=\"horizon-card-text-container\">\n        <div class=\"horizon-card-field-moon-phase\" style=\"transform: rotate(", "deg)\">\n          <ha-icon icon=\"mdi:", "\"></ha-icon>\n        </div>\n        <div class=\"horizon-card-field-value horizon-card-field-value-moon-phase\">", "</div>\n      </div>\n    "])), phaseRotation, phase.icon, moon_phase_localized);
    }