-
Notifications
You must be signed in to change notification settings - Fork 4.2k
feat(ui-mode): enhance status-line to show passed, failed, and skipped test counts #35859
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
base: main
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
7235701
to
8cf6a70
Compare
This comment has been minimized.
This comment has been minimized.
8cf6a70
to
3b6faae
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
3b6faae
to
986991e
Compare
This comment has been minimized.
This comment has been minimized.
986991e
to
62478dd
Compare
This comment has been minimized.
This comment has been minimized.
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.
Thank you for the PR. Some general comments:
- If you press the run button on a group of tests, the loading spinner will immediately appear on the left, but the current completed test count will still show the old value for a moment.
- The baseline on the stats is incorrect relative to the icons. This is probably an existing issue, but it becomes more obvious with this change.



I think this is good improvement and appreciate you looking into this.
@@ -461,11 +462,18 @@ export const UIModeView: React.FC<{}> = ({ | |||
runTests={() => runTests('bounce-if-busy', visibleTestIds)} /> | |||
<Toolbar noMinHeight={true}> | |||
{!isRunningTest && !progress && <div className='section-title'>Tests</div>} | |||
{!isRunningTest && progress && <div data-testid='status-line' className='status-line'> | |||
<div>{progress.passed}/{progress.total} passed ({(progress.passed / progress.total) * 100 | 0}%)</div> | |||
{!isRunningTest && progress && <div data-testid='status-line' className='status-line' title={`${progress.passed} passed, ${progress.failed} failed, ${progress.skipped} skipped`}> |
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 would probably set cursor: pointer
on this whole block, as selecting individual numbers in between the icons is never going to be good experience.
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.
Changed
.status-line {
cursor: pointer;
}
.status-line > span:not(:first-child) {
user-select: none;
}
.status-line
provides a title for the overall test status on hover.
To prevent text selection, user-select: none;
has been added to the individual test status elements.
Please let me know if this should be removed.
Image
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'm sorry, I have no idea why I said cursor: pointer
. I meant cursor: default
.
{!isRunningTest && progress && <div data-testid='status-line' className='status-line'> | ||
<div>{progress.passed}/{progress.total} passed ({(progress.passed / progress.total) * 100 | 0}%)</div> | ||
{!isRunningTest && progress && <div data-testid='status-line' className='status-line' title={`${progress.passed} passed, ${progress.failed} failed, ${progress.skipped} skipped`}> | ||
<span data-testid='test-count'>{progress.passed + progress.failed + progress.skipped}/{progress.total}</span> |
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.
We don't want this section to bounce in-between running and steady states. Classically we would display a green checkmark here, but I don't think that fits very well.
In absence of a better icon I propose codicon-circle-outline
.
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 className={clsx('codicon', isRunning ? testStatusIcon('running') : testStatusIcon('none'))} />
The changes have been applied. Thanks for the helpful review :)
@@ -461,11 +462,18 @@ export const UIModeView: React.FC<{}> = ({ | |||
runTests={() => runTests('bounce-if-busy', visibleTestIds)} /> | |||
<Toolbar noMinHeight={true}> | |||
{!isRunningTest && !progress && <div className='section-title'>Tests</div>} |
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 only gets displayed before you run any tests or on reload. Get rid of it and show an empty set of stats, with 0/n
on the left (where n
is your total active test count).
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.
62478dd
to
0b66a3e
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
aa8df54
to
3fe70d5
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1.
It seems that the current 2.
If you still see any issues, I’d appreciate your feedback. 3.
I checked the input and related CSS in 4.
Ellipsis is applied at the chunk level for each |
8aa5f2b
to
088476a
Compare
This comment has been minimized.
This comment has been minimized.
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.
Thank you for the thorough responses and images. Alignment looks good except for the ellipsis issue.
I agree that the blocked render fix should go in a future PR. Please open an issue to that effect once this merges.
*/ | ||
|
||
.status-line { | ||
display: block; |
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 should be display: flex
with align-items: center
. You'll have to fix some other things, but as it is the ellipsis is misaligned from the text baseline as it's not currently vertically centered.
088476a
to
63304b2
Compare
This comment has been minimized.
This comment has been minimized.
63304b2
to
84fc89c
Compare
I’d like to discuss the alignment of the ellipsis. 1.I wasn’t able to apply 2.The ellipsis( 3.I tried several fixes, and the most effective was using If you have any suggestions for resolving this, I’d appreciate your feedback. :) |
Test results for "tests 1"6 flaky39212 passed, 803 skipped Merge workflow run. |
Closes #34444 (assignee @agg23)
Implemented prior discussion and feedback
Looking forward to the team’s feedback. Thank you :)