-
-
Notifications
You must be signed in to change notification settings - Fork 288
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
feat(ecowatt): Add day in box display #1687
Conversation
Codecov ReportBase: 97.27% // Head: 97.27% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## master #1687 +/- ##
=======================================
Coverage 97.27% 97.27%
=======================================
Files 643 643
Lines 9686 9686
=======================================
Hits 9422 9422
Misses 264 264 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm good to merge, but I'm ok with this approach only because this integration is used in France only!
Creating custom dates format like that doesn't work with an international approach, because not all country put the day of the week first.
Just the english version of this is not strictly correct:
Should be "Mon, January 16, 2023"
For future integration, what I recommend:
- Either using official formats in dayjs
- Or putting the format in the translation so it can be modified for each language
So ok for this one, but only because it's a FR-only integration!
Totally agreeing, I took the fast path 😏 |
Yes, no worries for this PR, it's good enough like that! |
Job #994: Bundle Size — 7.01MiB (~+0.01%).
Metrics (1 change)
Total size by type (1 change)
|
DayJS proposes only 3 formats for day:
With the available space, I thought that last format was the best