-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
feat: use <progress> and <svg> for browser progress indicator instead of <canvas> #5015
feat: use <progress> and <svg> for browser progress indicator instead of <canvas> #5015
Conversation
I'll ask my employer about this easyCLA thing. Edit: I asked, waiting for response |
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 like this direction! In fact, I'd even say let's go with a full switch, rather than a fallback? 🔥
Sure, I can do that. The client that I was working with doesn't support canvas very well, but progress element is supported, so this does sound like a better solution. |
Just to be explicit: I don't think we should change the visual design in this PR. We should keep to the same progress visualization as was being done with canvas. |
Keeping both the <progress> element and the design will be tricky, but possible with multiple CSS transforms and progress elements to warp the bars into a circle. However, changing the rounded ends of the bar might need to use nonstandard CSS to achieve. We could instead replace element with HTML elements with JS to set CSS gradients to show progress. However, doing that would move up the browser version requirements: https://caniuse.com/?search=conic-gradient |
An alternative could be to use an As for the rounded bar ends, where do you see those? This is what I get locally: Screen.Recording.2024-03-07.at.3.08.16.PM.movNote the lack of animation or rounded-ness. |
hide progress bar and use SVG to recreate the ring bar
Just a heads up @yourWaifu, I merged the latest |
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.
🙌 Progress! This is getting pretty close to merge, I think. Thanks for all the iteration!
Requesting changes on a few things that changed unintentionally. Please make sure the new visual thing is as close to the same as the old one as is straightforward to go for, including approximate border widths, colors, and font styles.
Once you think it's ready for re-review please re-request my review (as in, with the re-request button on GitHub) and I'll be excited to take another look.
removed more canvas related code removed an em
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.
Cool! I think we can simplify the code here a bit more, and get the visual changes reduced. Progress! 🚀
renamed ring-whole to ring-flatlight and have ring highlight and flatlight be the inverse of each other fix spelling error with decimalPlaces use getComputedStyle for get the radius of the ring defined in CSS use :is() CSS to simplify CSS code Change stroke thickness math to better match previous look
Description of the Change
It's possible for HTMLCanvasElement to not be available. In the HTML reporter, if canvas isn't available, there's no fallback to see the progress percentage.
here's a picture:
Alternate Designs
Using CSS flex-basis to create a progress bar, this is good alternative and can also work. However, to ensure compatibility, I decided to use text as the fallback because I know it'll work.
Why should this be in core?
Seems to be a bug that makes it very hard to see the progress of test.
Benefits
The progress can be seen regardless if canvas is available
Possible Drawbacks
Very niche, 99% of cases, canvas is supported
Applicable issues
Fixes #5113.