-
Notifications
You must be signed in to change notification settings - Fork 72
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
removed moment as a dependency #6
Conversation
@skodamarthi this is ready for review now |
300ef10
to
73a305d
Compare
Moment is gone completely now. And I rebased since we have had a decent number of contributions since I branched. I also found a slightly cleaner way to pick out the bits of the formatted string we want... and added a comment besides to make it clearer when we look back in a years time. Ready for review and merge when you have some time @skodamarthi |
src/js/common/browserInfoHelper.js
Outdated
// nowUTC is in format like "Wed Sep 30 2020 23:11:02 GMT+0100 (British Summer Time)" | ||
// To format the offset, we pick the relevant characters based on their position | ||
// and reformat with a ":" | ||
const signCharIndex = 28; |
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 was thinking figuring out position 28, 29 etc; from such long string is difficult. How about splitting the string to bare minimum required (i.e +0100) and applying the logic to fetch hour and minute offset from that?
Eg: const nowUTC = new Date().toString().split('GMT')[1].split(' ')[0]; // gives "+0100"
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.
if we're just reading the timestamp from the current browser session, it's possible to use getTimezoneOffset?
const offset = new Date().getTimezoneOffset() * -1;
const offsetHours = `${Math.floor(offset / 60)}`.padStart(2, 0);
const offsetMinutes = `${Math.abs(offset % 60)}`.padStart(2, 0);
const sign = offset >= 0 ? '+' : '-'
const offsetString = `UTC${sign}${offsetHours}:${offsetMinutes}`
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.
if we're just reading the timestamp from the current browser session, it's possible to use getTimezoneOffset?
const offset = new Date().getTimezoneOffset() * -1; const offsetHours = `${Math.floor(offset / 60)}`.padStart(2, 0); const offsetMinutes = `${Math.abs(offset % 60)}`.padStart(2, 0); const sign = offset >= 0 ? '+' : '-' const offsetString = `UTC${sign}${offsetHours}:${offsetMinutes}`
While this would work it looks a good deal more complicated than the current version in the PR, which is based on @skodamarthi 's suggestion.
Title
Description of what's changing
Removed moment as a dependency to cut down package size
What else might be impacted?
...
Checklist
[ ] Tests are written and maintain or improve code coverage
[ ] I've tested this in my application using
yarn link
(if applicable)[ ] You consent to and are confident this change can be released with no regression