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

Add leading zero to hour & minute on <title /> when needed #314

Merged

Conversation

@mhxbe
Copy link
Contributor

@mhxbe mhxbe commented Sep 22, 2019

Hi,

Recently @gaokun requested/added a date in the <title /> tag which is a great addition to the tool.
Last night, when I was using webpack-bundle-analyzer on a personal project, I saw the following date:

<title>react-starter [21 Sep 2019 at 1:5]</title>

Problem
By default, .getHours() & .getMinutes() return a number which results in a one-digit for all numbers lower than 10. In this case, it's displaying 1:5 for the hour & minutes whereas 01:05 would be nicer.

Possible solution
Adding a leading zero in combination with converting it to a String, leverages us the usage of .slice(-2) so it returns the last 2 digits.

Another solution is the usage of .substr(-2) but it seems this method will be deprecated in the future
Source for .slice(): String.prototype.slice()
Source for .substr(): String.prototype.substr()

If there are better approaches, I'm open for them.

Examples
You can copy paste the following code-snippet in your browser console to see it in action.

const someDate = new Date('2019-09-21 01:05');

console.log('Hours:', someDate.getHours());
console.log('Minutes:', someDate.getMinutes());

console.log('With leading zeros:');
console.log('Hours:', `0${someDate.getHours()}`.slice(-2));
console.log('Minutes:', `0${someDate.getMinutes()}`.slice(-2));
…) to cut it off when not needed
@jsf-clabot
Copy link

@jsf-clabot jsf-clabot commented Sep 22, 2019

CLA assistant check
All committers have signed the CLA.

@gaokun
Copy link
Contributor

@gaokun gaokun commented Sep 22, 2019

Thanks for mention this. It is nice format to read.

And what about ES6 padStart function?

'4'.padStart(2, 0);  //  result: '04'

In this case, they are both OK, anyway.

@mhxbe
Copy link
Contributor Author

@mhxbe mhxbe commented Sep 22, 2019

That's even better and much cleaner, thanks for the suggestion.

I quickly checked the MDN docs and they state that .padStart() is part of ES2017.
This means it's covered with @babel/present-env but there's a possible catch: Internet Explorer-support.

Is it safe to say this will work in both cases (server & client) since src/viewer.js is 'server-side' code thus not affecting IE?

@th0r
Copy link
Collaborator

@th0r th0r commented Sep 22, 2019

We doesn't support IE11 in client bundle so it's completely safe to use padStart.

@mhxbe
Copy link
Contributor Author

@mhxbe mhxbe commented Sep 22, 2019

Ok, thanks for the quick response @th0r.

I've pushed another commit where I used template literals to convert the number to a String.

const hour = `${time.getHours()}`.padStart(2,0);
const minute = `${time.getMinutes()}`.padStart(2,0);

I can change this to the following if preferred:

const hour = time.getHours().toString().padStart(2,0);
const minute = time.getMinutes().toString().padStart(2,0);
@th0r
Copy link
Collaborator

@th0r th0r commented Sep 22, 2019

Tag templates are ok, but let's better change method calls to padStart(2, '0') to be more clear.

@th0r
Copy link
Collaborator

@th0r th0r commented Sep 23, 2019

According to tests, padStart is not available in Node v6. 😕
I don't want to add another runtime dependency to polyfill it so let's better use slice approach.

And there is also an ESLint error.

@valscion
Copy link
Member

@valscion valscion commented Oct 17, 2019

Hi @mhxbe, it would be great to get this bug fixed ☺️. Are you still up for this PR?

@mhxbe
Copy link
Contributor Author

@mhxbe mhxbe commented Oct 17, 2019

Hi @valscion, I've been busy past week(s) but I will get this fixed tonight.
It's possible I'll reopen a new PR because I've made some 'polluted' commits.

If you've got time to fix this now, feel free to open up a new PR with the fix. I'll happily close this one then. If this is not the case, I will fix this tonight.

@valscion
Copy link
Member

@valscion valscion commented Oct 17, 2019

Don't worry about the commit history ☺️ it's OK if there's some back-and-forth commits, as this is quite small PR still.

@mhxbe
Copy link
Contributor Author

@mhxbe mhxbe commented Oct 17, 2019

@valscion I pushed a commit which reverts the .padStart to .slice which fixes the build on Node 6. Thanks for the follow-up. 👍

valscion added 3 commits Oct 17, 2019
My merge conflict resolve had a hickup 😅
@valscion
Copy link
Member

@valscion valscion commented Oct 17, 2019

Ok this seems good to go to me! Thanks ☺️ I'll merge once CI is green

@valscion valscion added the type: Bug label Oct 17, 2019
@valscion valscion merged commit 7cefb58 into webpack-contrib:master Oct 17, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.