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

fix: plural of date picker #831

Merged
merged 3 commits into from
Feb 16, 2022
Merged

fix: plural of date picker #831

merged 3 commits into from
Feb 16, 2022

Conversation

louisphn
Copy link
Contributor

  • fix plural of datepicker (Last 1 hours -> Last 1 hour, Last 1 years -> Last 1 year)

Please take a look at the preview screenshots and let me know if there is other issues.
Thanks!

image
image

@CLAassistant
Copy link

CLAassistant commented Feb 14, 2022

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ louisphn
❌ Louis Phan


Louis Phan seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@louisphn
Copy link
Contributor Author

By the way because the CLA did not get updated so can I make use of this instead?
image

@petethepig
Copy link
Member

@louisphn Thanks for the contribution!

I think there's just an issue connecting your github account to the email. You need to either add the email you used to sign the CLA to your github account or sign it again with an email that is attached to your github account.

@codecov
Copy link

codecov bot commented Feb 14, 2022

Codecov Report

Merging #831 (ed00344) into main (bee6483) will decrease coverage by 5.65%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #831      +/-   ##
==========================================
- Coverage   73.09%   67.45%   -5.64%     
==========================================
  Files          65       81      +16     
  Lines        2311     2838     +527     
  Branches      421      545     +124     
==========================================
+ Hits         1689     1914     +225     
- Misses        594      896     +302     
  Partials       28       28              
Impacted Files Coverage Δ
webapp/javascript/util/formatDate.ts 89.14% <100.00%> (+24.69%) ⬆️
webapp/javascript/ui/Button.tsx 75.00% <0.00%> (-5.00%) ⬇️
webapp/javascript/services/base.ts 87.94% <0.00%> (-4.52%) ⬇️
webapp/javascript/redux/reducers/newRoot.ts 84.85% <0.00%> (-2.65%) ⬇️
webapp/javascript/components/ExportData.tsx 41.49% <0.00%> (-0.75%) ⬇️
webapp/javascript/models/users.ts 50.00% <0.00%> (ø)
...cript/components/FlameGraph/FlameGraphRenderer.jsx 45.54% <0.00%> (ø)
...pp/javascript/components/ProfilerTable.module.scss 53.85% <0.00%> (ø)
webapp/javascript/services/users.ts 20.78% <0.00%> (ø)
webapp/javascript/services/TestData.ts 100.00% <0.00%> (ø)
... and 17 more

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 bee6483...ed00344. Read the comment docs.

Copy link
Collaborator

@eh-am eh-am left a comment

Choose a reason for hiding this comment

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

Awesome, thank you so much for your contribution!

The only thing I would ask is if you can figure out why this test was NOT failing.
https://github.com/pyroscope-io/pyroscope/blob/main/webapp/javascript/util/formatDate.spec.ts#L10

@louisphn
Copy link
Contributor Author

@eh-am looks like the 'readableRange' test was skipped so it was not failing. When I changed test.each to test.only.each the test now works. Should I be pushing this to the PR?

image

image

Because it is test.only, all the other tests in file are skipped, except for the one with only like now-1h in formatAsObject test
image

@eh-am
Copy link
Collaborator

eh-am commented Feb 14, 2022

@louisphn ahh that makes sense!

I think it's easier to just remove this .only instead? https://github.com/pyroscope-io/pyroscope/blob/main/webapp/javascript/util/formatDate.spec.ts#L48

@louisphn
Copy link
Contributor Author

louisphn commented Feb 15, 2022

@eh-am true haha should have done that instead of adding only to each test
I fixed that in the latest commit so please take a look when you have time. (the test passed by the way)
Thanks!

@eh-am
Copy link
Collaborator

eh-am commented Feb 15, 2022

Almost there, the tests are still failing but that's probably my fault. Here's what you should do:

diff --git a/webapp/javascript/util/formatDate.spec.ts b/webapp/javascript/util/formatDate.spec.ts
index 3bfab904..f00dfb91 100644
--- a/webapp/javascript/util/formatDate.spec.ts
+++ b/webapp/javascript/util/formatDate.spec.ts
@@ -42,7 +42,7 @@ describe('FormatDate', () => {
     it('works with "now"', () => {
       // TODO
       // not entirely sure this case is even possible to happen in the code
-      expect(formatAsOBject('now')).toEqual(mockDate);
+      expect(formatAsOBject('now')).toEqual(mockDate.getTime());
     });
 
     it('works with "now-1h"', () => {
@@ -58,12 +58,12 @@ describe('FormatDate', () => {
     });
 
     it('works with "now-1m"', () => {
-      expect(formatAsOBject('now-1m')).toBe(1640090641741);
+      expect(formatAsOBject('now-1m')).toBe(1640090581741);
     });
 
     it('works with absolute timestamps', () => {
       expect(formatAsOBject('1624192489')).toEqual(
-        new Date('2021-06-20T12:34:49.000Z')
+        new Date('2021-06-20T12:34:49.000Z').getTime()
       );
     });
   });

Also it looks like you haven't signed yet, can you have a look at it?
#831 (comment)

Thank you again @louisphn!

@louisphn
Copy link
Contributor Author

@eh-am I have fixed according to your code. Please take a look when you have time. Thanks!
I have signed the CLA but the first commit was not linked to my email address at the time I pushed so it shows there are 2 committers which I don't know how to sign on behalf of the other username. Do you have any idea how to fix this?

@eh-am
Copy link
Collaborator

eh-am commented Feb 16, 2022

@petethepig can you shed some light RE the license thing?

@petethepig petethepig merged commit 8bd6eb8 into grafana:main Feb 16, 2022
korniltsev pushed a commit that referenced this pull request Jul 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants