Skip to content
This repository has been archived by the owner on Mar 23, 2023. It is now read-only.

refactor: replace moment with dayjs #1753

Merged
merged 13 commits into from
Mar 4, 2020
Merged

refactor: replace moment with dayjs #1753

merged 13 commits into from
Mar 4, 2020

Conversation

faustbrian
Copy link
Contributor

Replaces all occurrences of moment with dayjs.

@ghost ghost added Complexity: Low Less than 64 lines changed. Type: Refactor The pull request improves or enhances an existing implementation. labels Mar 2, 2020
@codecov
Copy link

codecov bot commented Mar 2, 2020

Codecov Report

Merging #1753 into develop will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1753      +/-   ##
===========================================
- Coverage    59.90%   59.89%   -0.02%     
===========================================
  Files          365      366       +1     
  Lines         9136     9145       +9     
  Branches      1847     1848       +1     
===========================================
+ Hits          5473     5477       +4     
- Misses        3483     3486       +3     
- Partials       180      182       +2     
Impacted Files Coverage Δ
src/renderer/utils/index.js 55.55% <0.00%> (-44.45%) ⬇️
src/renderer/services/datetime.js 100.00% <0.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 18ead7d...ed6e52a. Read the comment docs.

Comment on lines +40 to +48
if (userLanguage === 'en-us') {
userLanguage = 'en'
}

try {
require(`dayjs/locale/${userLanguage}`)
} catch {
userLanguage = 'en'
}
Copy link
Contributor

@dated dated Mar 2, 2020

Choose a reason for hiding this comment

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

More generic:

Suggested change
if (userLanguage === 'en-us') {
userLanguage = 'en'
}
try {
require(`dayjs/locale/${userLanguage}`)
} catch {
userLanguage = 'en'
}
const [language, region] = userLanguage.split('-')
if (
(language === 'en' && region === 'us') ||
language === region
) {
userLanguage = language
}
try {
require(`dayjs/locale/${userLanguage}`)
} catch {
userLanguage = 'en'
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This will also handle locales like de-DE, it-IT ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also more expensive due to how require works inside of electron.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gimme a minute, I'll boil it down to one require

Copy link
Contributor

@dated dated Mar 2, 2020

Choose a reason for hiding this comment

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

Updated above

@alexbarnsley
Copy link
Member

moment is used in src/renderer/services/crypto/transaction-signer.js since a different PR was merged

@alexbarnsley alexbarnsley merged commit 7479bf3 into develop Mar 4, 2020
@ghost ghost deleted the refactor/moment branch March 4, 2020 11:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Complexity: Low Less than 64 lines changed. Type: Refactor The pull request improves or enhances an existing implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants