-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
issue-6168: added leading zero to test runner counter in stats.tsx file #6236
issue-6168: added leading zero to test runner counter in stats.tsx file #6236
Conversation
Thanks for the contribution! Below are some guidelines Cypress uses when doing PR reviews.
PR Review ChecklistIf any of the following requirements can't be met, leave a comment in the review selecting 'Request changes', otherwise 'Approve'. User Experience
Functionality
Maintainability
Quality
Internal
|
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.
@Manuel-Suarez-Abascal Thanks for the contribution! Could you please sign our CLA?
You're welcome @jennifer-shehane! I have signed the CLA. Let me know, if you have any questions about my implementation! |
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.
@Manuel-Suarez-Abascal There's a failing unit test in the file below. Can you address this?
1) <Stats />
duration
renders -- when zero:
AssertionError: expected the node in <_class /> to have text '--', but it has '000--'
HTML:
<span className="num">000--</span>
+ expected - actual
-000--
+--
at Context.<anonymous> (src/header/stats.spec.tsx:64:56)
at processImmediate (internal/timers.js:439:21)
@jennifer-shehane I have addressed the failing unit test. The solution has passed all tests now! |
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.
This is great. Thanks for addressing the test failure also. 💯
Why not add a min-width and right-align the text? the leading zero only adds visual clutter if you want you can use |
The leading zero is only up until 10 secs and they are using .toFixed(2) to force 2 decimal places. I think this will look better than min-width with a weird space on the left. |
User facing changelog
The timer in the Test Runner now always displays at a consistent length.
Additional Details
I opened this PR to fix issue-6268. I solved the jumpy bug on the runner test timer by adding a leading zero to the header counter.
How has the user experience changed?
PR Tasks
cypress-documentation
?