-
Notifications
You must be signed in to change notification settings - Fork 427
fontend: Loader: Some a11y fixes and platform fix #3796
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: illume The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This avoids SVG that mui CircleProgress uses, so it doesn't freeze. Also avoids flashing if the loading is finished before 0.25 seconds. Fixes a11y issue to not use progressbar, because we always do not know the progress, but instead are announcing a status. Removes test which checks that people can add other props to it, because no one is using this functionality. Color and size props are used, and are still supported.
So it looks a bit nicer, and shows more there. This is similar to how it's done in many places like the storybook loader for example.
This makes the loader spin a lot slower, with pauses.
/** | ||
* Injects custom loader styles into the document, once for each theme. | ||
*/ | ||
function injectLoaderStyle(theme: any) { |
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.
why do styles need to be injected like this?
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.
What’s your problem with it?
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.
why not <Box sx={...} />
?
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're using react so we don't have to deal with manually creating dom nodes/styles.
this kind of manual style injection is going to be harder to maintain, prone to bugs, less readable and unnecessary since we have nice abstractions like sx prop. or alternatively you could write a .css
file and import it.
I wanted to ask you first, maybe you had some reason to apply styles like this
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.
Pull Request Overview
This pull request refactors the Loader component to improve accessibility, prevent visual flashing, and fix platform-specific freezing issues. The changes replace MUI's CircularProgress with a custom CSS-based loader implementation.
- Replaces SVG-based loader with CSS animations to prevent freezing on busy main threads
- Adds accessibility improvements by changing role from "progressbar" to "status" and including aria-live
- Implements reduced motion support with slower animations that include pauses
- Adds a 0.25-second delay to prevent loader flashing for quick operations
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
File | Description |
---|---|
frontend/src/components/common/Loader.tsx |
Complete rewrite implementing custom CSS-based loader with accessibility and reduced motion support |
frontend/src/components/common/Loader.test.tsx |
Updated tests to reflect role changes from "progressbar" to "status" and removed obsolete prop tests |
Various snapshot files | Updated snapshots showing the new HTML structure with custom CSS classes and improved accessibility attributes |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
@@ -15,17 +15,169 @@ | |||
*/ | |||
|
|||
import Box from '@mui/material/Box'; | |||
import CircularProgress, { CircularProgressProps } from '@mui/material/CircularProgress'; | |||
import { alpha, useTheme } from '@mui/material/styles'; |
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.
The filename in the title contains a typo: 'fontend' should be 'frontend'.
Copilot uses AI. Check for mistakes.
/** | ||
* Injects custom loader styles into the document, once for each theme. | ||
*/ | ||
function injectLoaderStyle(theme: any) { |
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.
The theme parameter should be properly typed instead of using 'any'. Consider using the MUI Theme type or extract the required properties for better type safety.
function injectLoaderStyle(theme: any) { | |
function injectLoaderStyle(theme: Theme) { |
Copilot uses AI. Check for mistakes.
* @returns a color value based on the theme and the provided color prop. | ||
* If no color is provided, it defaults to the primary color of the theme. | ||
*/ | ||
function getColor(theme: any, color?: LoaderProps['color']) { |
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.
The theme parameter should be properly typed instead of using 'any'. Consider using the MUI Theme type for better type safety.
function getColor(theme: any, color?: LoaderProps['color']) { | |
function getColor(theme: Theme, color?: LoaderProps['color']) { |
Copilot uses AI. Check for mistakes.
}, [theme]); | ||
const loaderColor = getColor(theme, color); | ||
const muiColorClass = getMuiColorClass(color); | ||
let fadedLoaderColor = React.useMemo(() => withOpacity(loaderColor, 0.1), [loaderColor, theme]); |
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.
The dependency array includes 'theme' but the useMemo callback only uses 'loaderColor'. The theme dependency is unnecessary since loaderColor already captures the theme-dependent value.
let fadedLoaderColor = React.useMemo(() => withOpacity(loaderColor, 0.1), [loaderColor, theme]); | |
let fadedLoaderColor = React.useMemo(() => withOpacity(loaderColor, 0.1), [loaderColor]); |
Copilot uses AI. Check for mistakes.
How to test