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.

Sign up for GitHub

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

Jump to bottom

feat: highlight browser failures #5218

Closed
wants to merge 18 commits into from
Closed

feat: highlight browser failures #5218

wants to merge 18 commits into from

Conversation

mark-wiemer
Copy link
Contributor

@mark-wiemer mark-wiemer commented Sep 29, 2024 •
edited
Loading

Picking up where #4148 left off as I don't have push permissions to that fork.

  • Adds red outline to status bar when any test fails
  • Adds colored checkmark or X to status bar based on whether tests have passed or failed (no text until root suite is complete)
  • Pushes down heading slightly to make room for the taller status bar due to minor style issues with the progress circle

Closes #792

Pass Fail
Dark dark pass dark fail
Light light pass light fail

@dhuang612
squashing extra commits
c9b2939 
setting package-lock.json back to last update

added in sass and updated css class to use extend

updated test files to expect sass
@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Sep 29, 2024 •
edited
Loading

CLA Not Signed

mark-wiemer
mark-wiemer commented Sep 29, 2024
View reviewed changes
mocha.sass Outdated Show resolved Hide resolved
@mark-wiemer mark-wiemer mentioned this pull request Sep 29, 2024
Closed
mark-wiemer
mark-wiemer commented Sep 29, 2024
View reviewed changes
Copy link
Contributor Author

@mark-wiemer mark-wiemer left a comment

Choose a reason for hiding this comment

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

Pending changes from original PR

mocha.sass Outdated Show resolved Hide resolved
@mark-wiemer
Copy link
Contributor Author

FYI, I'm very new to contributing to mocha, so right now I'm still trying to just reproduce the issue. I've installed Chrome and npm run test-browser-run is passing, which is great, but I don't know yet how to keep the browser open to inspect test results. I always use the CLI :D

@mark-wiemer mark-wiemer changed the title Highlight browser failures feat: Highlight browser failures Sep 29, 2024
@mark-wiemer mark-wiemer changed the title feat: Highlight browser failures feat: highlight browser failures Sep 29, 2024
@mark-wiemer
Copy link
Contributor Author

image

Still struggling to get a passing case working with a local build of mocha.js, I'm only used to the default CLI reporter! But I think outline is a better way to go than background color, and some clear text/shape indicating pass or fail will work for colorblind folks as well. Not sure if we want to add an announcement to the HTML reporter if tests fail, but that sounds beyond the scope of this PR anyway.

I'll continue working to get a passing example, fixup padding, and make any requested changes as well :)

@mark-wiemer
Copy link
Contributor Author

The styles are kinda tough with display: block, I'm going to see if I can change to display: flex. Would that be OK or is it strongly not recommended?

Here is the current appearance, btw:

image

image

image

image

@mark-wiemer
Copy link
Contributor Author

Was able to make things work with display: block;, if a slightly taller header is OK. Final styles until review:

image

image

image

image

@mark-wiemer mark-wiemer mentioned this pull request Sep 30, 2024
Closed
JoshuaKGoldberg
JoshuaKGoldberg requested changes Oct 8, 2024
View reviewed changes
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Seems like a reasonable implementation for the proposed design at first. But I think we'll want to discuss a bit before settling on the design. Thanks for sending! ❤️

mocha.css
@@ -352,7 +377,7 @@ body {
#mocha-stats :is(.progress-element, .progress-text) {
width: var(--ring-container-size);
display: block;
top: 12px;
top: 17px;
Copy link
Member

Choose a reason for hiding this comment

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

😬 I was really hoping we wouldn't move UI elements around for this change. The taller this section, the less fits on the page in smaller viewports. A lot of people are accustomed to the placement as-is. Which is why I was hoping to discuss a bit in the backing issue (#4148 (review)) before having a PR author such as yourself go through the effort of implementing a more complex design. I'll post there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries, just wanted to get something looking good, let's discuss in the backing issue :)

@JoshuaKGoldberg JoshuaKGoldberg added the status: waiting for author waiting on response from OP - more information needed label Oct 8, 2024
@mark-wiemer mark-wiemer closed this by deleting the head repository Oct 10, 2024
@mark-wiemer
Copy link
Contributor Author

Sorry, I had forked a fork instead of the main repo, it was becoming difficult to manage. Will open this as a separate PR shortly, don't worry :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoshuaKGoldberg JoshuaKGoldberg JoshuaKGoldberg requested changes

Assignees
No one assigned
Labels
status: waiting for author waiting on response from OP - more information needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

🚀 Feature: Highlight browser failures in red
3 participants
@mark-wiemer @JoshuaKGoldberg @dhuang612