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

Fix moon phase integration #131

Open
wants to merge 57 commits into
base: main
Choose a base branch
from
Open

Fix moon phase integration #131

wants to merge 57 commits into from

Conversation

MelleD
Copy link

@MelleD MelleD commented Apr 26, 2024

Pull Request Template for Home Assistant / Lovelace Card Repository

Overview

a) Fixed issue #122
b) Fixed or ignored broken test for language for persia fa
c) Add new release pipeline

Type of Change

  • Bugfix
  • New feature
  • Enhancement
  • Documentation update
  • Refactoring
  • Other (please describe):

Copy link

@ThomDietrich ThomDietrich left a comment

Choose a reason for hiding this comment

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

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

- dependencies
- title: ⭐ New Features
labels:
- "*"

Choose a reason for hiding this comment

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

Why do we need this file?

Copy link
Author

@MelleD MelleD Apr 27, 2024

Choose a reason for hiding this comment

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

It's for the github release page. See example here
https://github.com/MelleD/pm-index-card/releases/tag/0.2.0
or here
MelleD#1

Choose a reason for hiding this comment

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

Not sure if this is relevant to fixing the moon phase integration?

@@ -51,7 +51,7 @@
"dependencies": {
"custom-card-helpers": "^1.8.0",
"lit": "^2.7.2",
"suncalc3": "link:suncalc3"
"suncalc3": "^2.0.5"

Choose a reason for hiding this comment

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

What's the effect of this change?

Copy link
Author

Choose a reason for hiding this comment

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

I couldn't install link:suncalc3 locally. I got an error: "npm ERR! Unsupported URL Type "link:": link:suncalc3"

So now install the latest version
https://www.npmjs.com/package/suncalc3

@@ -59,8 +59,11 @@ describe('I18N', () => {
const date = new Date('2023-04-13T00:35:46.789Z')
const result = i18n.formatDateAsTime(date)

const timeRegex = /0?0[:.]35|12[:.]35/
expect(result).toMatch(timeRegex)
if( language != "fa"){

Choose a reason for hiding this comment

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

I don't like this hidden bug. Let's rather just "disable" the fa translation so someone can come in and fix it.

Copy link
Author

Choose a reason for hiding this comment

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

Not sure if this is really a bug, because something is translated

Copy link
Author

Choose a reason for hiding this comment

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

Revert it and changed the encoding of the file to UTF-8

@ThomDietrich
Copy link

ThomDietrich commented Apr 27, 2024

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.

@MelleD
Copy link
Author

MelleD commented Apr 27, 2024

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 and others added 4 commits April 27, 2024 07:56
@MelleD
Copy link
Author

MelleD commented Apr 27, 2024

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

MelleD and others added 10 commits April 27, 2024 08:17
…rse-7.24.1

Bump @babel/traverse from 7.21.4 to 7.24.1
Bumps [word-wrap](https://github.com/jonschlinkert/word-wrap) from 1.2.3 to 1.2.5.
- [Release notes](https://github.com/jonschlinkert/word-wrap/releases)
- [Commits](jonschlinkert/word-wrap@1.2.3...1.2.5)

---
updated-dependencies:
- dependency-name: word-wrap
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [semver](https://github.com/npm/node-semver) from 6.3.0 to 6.3.1.
- [Release notes](https://github.com/npm/node-semver/releases)
- [Changelog](https://github.com/npm/node-semver/blob/v6.3.1/CHANGELOG.md)
- [Commits](npm/node-semver@v6.3.0...v6.3.1)

---
updated-dependencies:
- dependency-name: semver
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [tough-cookie](https://github.com/salesforce/tough-cookie) from 4.1.2 to 4.1.3.
- [Release notes](https://github.com/salesforce/tough-cookie/releases)
- [Changelog](https://github.com/salesforce/tough-cookie/blob/master/CHANGELOG.md)
- [Commits](salesforce/tough-cookie@v4.1.2...v4.1.3)

---
updated-dependencies:
- dependency-name: tough-cookie
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [@rollup/plugin-babel](https://github.com/rollup/plugins/tree/HEAD/packages/babel) from 6.0.3 to 6.0.4.
- [Changelog](https://github.com/rollup/plugins/blob/master/packages/babel/CHANGELOG.md)
- [Commits](https://github.com/rollup/plugins/commits/babel-v6.0.4/packages/babel)

---
updated-dependencies:
- dependency-name: "@rollup/plugin-babel"
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
…e-4.1.3

Bump tough-cookie from 4.1.2 to 4.1.3
…in-babel-6.0.4

Bump @rollup/plugin-babel from 6.0.3 to 6.0.4
directory: "/"
# Check the npm registry for updates every day (weekdays)
schedule:
interval: "daily"
Copy link
Author

Choose a reason for hiding this comment

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

This for dependabot to create PR see example here
MelleD#1

You have to enable it under settings

@ThomDietrich
Copy link

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 and others added 12 commits May 2, 2024 23:37
…-5.4.5

Bump typescript from 5.0.4 to 5.4.5
…onment-jsdom-29.7.0

Bump jest-environment-jsdom from 29.5.0 to 29.7.0
…et-env-7.24.5

Bump @babel/preset-env from 7.21.4 to 7.24.5
Bumps [ts-jest](https://github.com/kulshekhar/ts-jest) from 29.1.0 to 29.1.2.
- [Release notes](https://github.com/kulshekhar/ts-jest/releases)
- [Changelog](https://github.com/kulshekhar/ts-jest/blob/main/CHANGELOG.md)
- [Commits](kulshekhar/ts-jest@v29.1.0...v29.1.2)

---
updated-dependencies:
- dependency-name: ts-jest
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/eslint-plugin) from 5.57.1 to 5.62.0.
- [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases)
- [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/CHANGELOG.md)
- [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v5.62.0/packages/eslint-plugin)

---
updated-dependencies:
- dependency-name: "@typescript-eslint/eslint-plugin"
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [@babel/core](https://github.com/babel/babel/tree/HEAD/packages/babel-core) from 7.24.4 to 7.24.5.
- [Release notes](https://github.com/babel/babel/releases)
- [Changelog](https://github.com/babel/babel/blob/main/CHANGELOG.md)
- [Commits](https://github.com/babel/babel/commits/v7.24.5/packages/babel-core)

---
updated-dependencies:
- dependency-name: "@babel/core"
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [@rollup/plugin-commonjs](https://github.com/rollup/plugins/tree/HEAD/packages/commonjs) from 24.0.1 to 25.0.7.
- [Changelog](https://github.com/rollup/plugins/blob/master/packages/commonjs/CHANGELOG.md)
- [Commits](https://github.com/rollup/plugins/commits/commonjs-v25.0.7/packages/commonjs)

---
updated-dependencies:
- dependency-name: "@rollup/plugin-commonjs"
  dependency-type: direct:development
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>
…gin-commonjs-25.0.7

Bump @rollup/plugin-commonjs from 24.0.1 to 25.0.7
…-7.24.5

Bump @babel/core from 7.24.4 to 7.24.5
…-eslint/eslint-plugin-5.62.0

Bump @typescript-eslint/eslint-plugin from 5.57.1 to 5.62.0
@MelleD
Copy link
Author

MelleD commented May 4, 2024

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.

dependabot bot and others added 8 commits May 6, 2024 18:59
Bumps [lit](https://github.com/lit/lit/tree/HEAD/packages/lit) from 2.7.2 to 3.1.3.
- [Release notes](https://github.com/lit/lit/releases)
- [Changelog](https://github.com/lit/lit/blob/main/packages/lit/CHANGELOG.md)
- [Commits](https://github.com/lit/lit/commits/[email protected]/packages/lit)

---
updated-dependencies:
- dependency-name: lit
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [@rollup/plugin-json](https://github.com/rollup/plugins/tree/HEAD/packages/json) from 6.0.0 to 6.1.0.
- [Changelog](https://github.com/rollup/plugins/blob/master/packages/json/CHANGELOG.md)
- [Commits](https://github.com/rollup/plugins/commits/url-v6.1.0/packages/json)

---
updated-dependencies:
- dependency-name: "@rollup/plugin-json"
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [@typescript-eslint/parser](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/parser) from 5.57.1 to 5.62.0.
- [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases)
- [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/parser/CHANGELOG.md)
- [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v5.62.0/packages/parser)

---
updated-dependencies:
- dependency-name: "@typescript-eslint/parser"
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [eslint-plugin-simple-import-sort](https://github.com/lydell/eslint-plugin-simple-import-sort) from 10.0.0 to 12.1.0.
- [Changelog](https://github.com/lydell/eslint-plugin-simple-import-sort/blob/main/CHANGELOG.md)
- [Commits](lydell/eslint-plugin-simple-import-sort@v10.0.0...v12.1.0)

---
updated-dependencies:
- dependency-name: eslint-plugin-simple-import-sort
  dependency-type: direct:development
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>
…gin-simple-import-sort-12.1.0

Bump eslint-plugin-simple-import-sort from 10.0.0 to 12.1.0
…-eslint/parser-5.62.0

Bump @typescript-eslint/parser from 5.57.1 to 5.62.0
…gin-json-6.1.0

Bump @rollup/plugin-json from 6.0.0 to 6.1.0
@ThomDietrich
Copy link

Hey Melle,
it is common practice to address one concern at a time.
#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
Copy link
Author

MelleD commented May 21, 2024

@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

@MelleD MelleD closed this May 21, 2024
@ThomDietrich ThomDietrich reopened this May 21, 2024
@ThomDietrich
Copy link

ThomDietrich commented May 21, 2024

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
Copy link

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
Copy link

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);
    }

@ThomDietrich ThomDietrich mentioned this pull request Oct 31, 2024
6 tasks
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.

5 participants