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

[CORE] Add more units #1479

Merged
merged 13 commits into from
May 3, 2022
Merged

[CORE] Add more units #1479

merged 13 commits into from
May 3, 2022

Conversation

VonOx
Copy link
Contributor

@VonOx VonOx commented Mar 22, 2022

Pull Request check-list

To ensure your Pull Request can be accepted as fast as possible, make sure to review and check all of these items:

  • [ ] If your changes affects code, did your write the tests?
  • Are tests passing? (npm test on both front/server)
  • Is the linter passing? (npm run eslint on both front/server)
  • Did you run prettier? (npm run prettier on both front/server)
  • If you are adding a new features/services, did you run integration comparator? (npm run compare-translations on front)
  • [ ] Did you test this pull request in real life? With real devices? If this development is a big feature or a new service, we recommend that you provide a Docker image to the community for testing before merging.
  • [ ] If your changes modify the API (REST or Node.js), did you modify the API documentation? (Documentation is based on comments in code)
  • [ ] If you are adding a new features/services which needs explanation, did you modify the user documentation? See the GitHub repo and the website.
  • [ ] Did you add fake requests data for the demo mode (front/src/config/demo.js) so that the demo website is working without a backend? (if needed) See https://demo.gladysassistant.com.

NOTE: these things are not required to open a PR and can be done afterwards / while the PR is open.

Description of change

Just adding some units asked by community and needed for future integrations ( Z2M Lixee TIC 👀 )

@codecov
Copy link

codecov bot commented Mar 22, 2022

Codecov Report

Merging #1479 (1807bbb) into master (9f64e60) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1479   +/-   ##
=======================================
  Coverage   96.58%   96.58%           
=======================================
  Files         617      617           
  Lines        8921     8921           
=======================================
  Hits         8616     8616           
  Misses        305      305           
Impacted Files Coverage Δ
server/utils/constants.js 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9f64e60...1807bbb. Read the comment docs.

@VonOx VonOx marked this pull request as ready for review March 22, 2022 14:25
@VonOx VonOx requested a review from Pierre-Gilles March 22, 2022 14:25
atrovato
atrovato previously approved these changes Mar 22, 2022
Copy link
Contributor

@Pierre-Gilles Pierre-Gilles left a comment

Choose a reason for hiding this comment

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

Nice PR, thanks @VonOx !

There are some translations missing in the i18n files, so those attributes could be used in the UI (in the MQTT integration for example)

front/src/config/i18n/en.json Show resolved Hide resolved
front/src/config/i18n/fr.json Show resolved Hide resolved
Copy link
Contributor Author

@VonOx VonOx left a comment

Choose a reason for hiding this comment

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

@Pierre-Gilles deviceFeatureCategory is already filled 😕

image

@Pierre-Gilles
Copy link
Contributor

@VonOx Weird, because I'm not able to use those devices in the MQTT integration. Is it working on your side ?

@VonOx
Copy link
Contributor Author

VonOx commented Apr 5, 2022

I miss some mapping in front/server, it's now better

@VonOx VonOx requested a review from Pierre-Gilles April 6, 2022 10:33
@VonOx
Copy link
Contributor Author

VonOx commented Apr 22, 2022

@Pierre-Gilles Can you look again please ?

@Terdious
Copy link
Contributor

Terdious commented May 3, 2022

Hello @VonOx,
To see how to turn the thing, but in connection with the heating, could you add a category "Time" which will be a command and return of state. The goal is to be able to display it either in date+hour+minute or in duration (declared in minutes for example) if > 60 then hours are displayed, otherwise minutes.
So for types:

  • a duration in integer I think (the goal here being to be able to display the remaining duration, to increase it or decrease it)
  • dateTimeUTC (format 1651346855 for example - the goal here being to display the end date without possible command? or add this one in the time-sensor)
    No extra unit I guess.
    Tell me what you think of it.
    to be able to display something like this:
    image

@VonOx
Copy link
Contributor Author

VonOx commented May 3, 2022

Hi @Terdious

  • The first one is already added ( I think ? ) , but duration seems to be a better term
"time-sensor": {
      "shortCategoryName": "Time sensor",
      "decimal": "Time (decimal)",
      "integer": "Time (integer)"
    }

Combined with unit of your choice

{
    "microseconds": "μs",
    "milliseconds": "ms",
    "seconds": "s",
    "minutes": "min",
    "hours": "h",
    "days": "j",
    "weeks": "sem",
    "months": "m",
    "years": "an."
}
  • The second one must be calculated I think (dayjs will do the math) , there is no device feature for that ?

Copy link
Contributor

@Pierre-Gilles Pierre-Gilles left a comment

Choose a reason for hiding this comment

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

Great PR! It'll really help users :)

I have a few changes.

Don't hesitate to open Gladys when doing this, then going to the MQTT integration, and see how it's looking in the UI, it should be easily searchable and understandable.

front/src/config/i18n/fr.json Outdated Show resolved Hide resolved
front/src/config/i18n/fr.json Outdated Show resolved Hide resolved
front/src/config/i18n/en.json Outdated Show resolved Hide resolved
front/src/config/i18n/fr.json Outdated Show resolved Hide resolved
front/src/config/i18n/fr.json Show resolved Hide resolved
@Pierre-Gilles
Copy link
Contributor

@Terdious I don't think we want to add this in this PR. It looks like there is some work to think about on heating devices, and if we include this development in this PR, this PR will never go out now!

@VonOx VonOx requested a review from Pierre-Gilles May 3, 2022 13:26
Copy link
Contributor

@Pierre-Gilles Pierre-Gilles left a comment

Choose a reason for hiding this comment

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

Ca me parait bien !

@Pierre-Gilles Pierre-Gilles merged commit 5ca643f into GladysAssistant:master May 3, 2022
@relativeci
Copy link

relativeci bot commented May 3, 2022

Job #276: Bundle Size — 6.83MB (+0.21%).

5ca643f vs bc12998

Changed metrics (2/10)
Metric Current Baseline
Initial JS 2.94MB(+0.49%) 2.92MB
Cache Invalidation 42.75% 0%
Changed assets by type (1/7)
            Current     Baseline
JS 4.92MB (+0.29%) 4.91MB

View Job #276 report on app.relative-ci.com

NickDub-old pushed a commit to NickDub/Gladys that referenced this pull request May 6, 2022
Co-authored-by: Pierre-Gilles Leymarie <pierregilles.leymarie@gmail.com>
@VonOx VonOx deleted the add-unit branch May 31, 2022 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants