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

HTML Reporter: headers fixed, tests scrollable #1513

Merged
merged 14 commits into from
Nov 30, 2020

Conversation

BryanCrotaz
Copy link
Contributor

@BryanCrotaz BryanCrotaz commented Nov 18, 2020

CSS change to keep test headers fixed at the top and make test results scrollable. Much improved dev experience.

Fixes #1512

image

@bertdeblock
Copy link

Nice! This has been bugging me as well. Though it seems that QUnit follows jQuery's browser support policy, which still includes IE9+ among others. I'm not 100% sure, but I'm guessing CSS features like calc, vw, vh and flex related styles might cause issues for older browsers?

@BryanCrotaz
Copy link
Contributor Author

BryanCrotaz commented Nov 19, 2020

How many developers use qunit in IE9? I could understand if it was going out to the public but this is for devs.

@bertdeblock
Copy link

Maybe there are still teams out there that run their tests in older browsers as part of their CI/CD flow? It seems there have been CSS adjustments in the past to fix issues, so it doesn't seem that far-fetched to me that adding new styles should take into account the browser support policy. I could be wrong of course and if so, I apologise in advance. I'm sure someone of the QUnit team can confirm. Thanks for the fix anyways, will really improve the DX 👍.

@BryanCrotaz
Copy link
Contributor Author

removed flexbox, vh and calc. Not responsive to header height any more. Suggest that flexbox is reinstated once the requirement to support IE9 is removed.

@BryanCrotaz
Copy link
Contributor Author

BryanCrotaz commented Nov 19, 2020

I have signed the licence but it says I haven't...
image

@BryanCrotaz
Copy link
Contributor Author

run their tests in older browsers as part of their CI/CD flow

Ah yes of course

@smcclure15
Copy link
Member

I can't stress enough how UX-friendly of a fix this is - I can certainly commiserate. Thanks for contributing!

The hardcoded pixel values are going to be hard to remain robust and flexible. I gave it bash with different window widths, and some get pretty due to the different line-wrapping:

image

image

image

image

@BryanCrotaz
Copy link
Contributor Author

The hardcoded pixel values are going to be hard to remain robust and flexible

Absolutely - that's why I'd like to use flexbox. Frankly if someone is using this in CI in IE9 they don't care about the UX, only the results. Can we use flexbox and have a fallback to the old behaviour?

Copy link
Member

@Krinkle Krinkle left a comment

Choose a reason for hiding this comment

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

I like this a lot!

Would it be possible to achieve this in a way that we only add styles to our own qunit- elements? In particular, styling the body element tends to cause test failures for integration tests that may temporarily render a UI component in a fixture or at the end of the body.

@BryanCrotaz
Copy link
Contributor Author

Would it be possible to achieve this

Yes. See new commit.

@BryanCrotaz
Copy link
Contributor Author

The hardcoded pixel values are going to be hard to remain robust and flexible

I think that the best solution is flexbox - it will fall back to the current layout in IE9 - that's not broken, it's just not better.

@BryanCrotaz
Copy link
Contributor Author

BryanCrotaz commented Nov 22, 2020

Here's the final version - does nothing if flexbox isn't supported, so has no effect on old browsers using @supports.

@Krinkle
Copy link
Member

Krinkle commented Nov 22, 2020

Noticed two issues when trying this out in the qunitjs.com demo.

Before After
a b
  1. The pass/fail banner gets reduced to 1px due to it having no content and thus the default flex shrinking behaviour reduces its height. Technically scrollable, but not useful.
  2. At short viewports, the scrollable area can be below the fold and thus unreachable (<360px), but even if it's somewhere between 360 and 500, it is still relatively difficult to read and scroll.

Perhaps we could wrap these rules in a media query for min-height: 500px. The banner issue could be addressed by something like #qunit-banner { flex: 5px 0 0; }

@BryanCrotaz
Copy link
Contributor Author

@Krinkle good spot, fixed

@BryanCrotaz
Copy link
Contributor Author

Test failure seems unrelated to the changes

@Krinkle
Copy link
Member

Krinkle commented Nov 29, 2020

  • Used tabs for consistency with the rest of the file.
  • Restored relation between Font Family and Sizes comment and its related code.
@Krinkle
Copy link
Member

Krinkle commented Nov 29, 2020

@BryanCrotaz This is good to go, but for the CLA signature pending.

@BryanCrotaz
Copy link
Contributor Author

@BryanCrotaz This is good to go, but for the CLA signature pending.

I've signed it but it still says it's pending. When I click on the pending notice it takes me to the form and says I've already signed it. Where do I go from here?

…html-reporter-experience

* commit '39dc8a53e90dd91c4cb30582ddc56585f2e3739b':
  Update qunit.css
  Update qunit.css
  ignore on small screens
@Krinkle
Copy link
Member

Krinkle commented Nov 30, 2020

Looks good now. Not sure what it was... Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants