Fix axe audits - #262
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request focuses on improving accessibility compliance by addressing axe audit findings. The changes enhance screen reader support through comprehensive aria-label attributes, add keyboard navigation aids with skip-to-content links and semantic landmarks, improve color contrast for better readability, and modernize the codebase by refactoring the CurrencyInput component from a class to a functional component with hooks. Additionally, dynamic currency support is introduced to respect user settings.
- Added aria-label attributes to icon-only buttons across the application for screen reader accessibility
- Implemented semantic HTML5 landmarks (main element with skip link) and improved color contrast ratios
- Refactored CurrencyInput from class-based to functional component using React hooks and added dynamic currency support via Redux
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/store/reducers/setting.ts | Adds getCurrencyAlpha3 selector to retrieve user's currency setting |
| src/locales/en.ts | Adds locale keys for user creation modal and submit button accessibility labels |
| src/components/user/create-user-inline-form.tsx | Adds aria-label to create user modal trigger and submit buttons |
| src/components/transaction/create-custom-transaction-form.tsx | Adds aria-label to deposit and dispense transaction buttons |
| src/components/settings/scaling-buttons.tsx | Adds aria-label to text size increase/decrease buttons |
| src/components/currency/currency.tsx | Adds dynamic currency support using Redux selector with optional override |
| src/components/currency/currency-input.tsx | Refactors component from class to functional component using hooks |
| src/components/currency/tests/currency.spec.tsx | Updates tests to include Redux Provider for currency selector |
| src/components/currency/tests/currency-input.spec.tsx | Updates tests to include Redux Provider wrapper |
| src/components/article/article-list.tsx | Adds aria-label to add article button |
| src/bricks/theme/theme.css | Improves text contrast colors and adds visually-hidden utility class for screen readers |
| src/bricks/theme/theme-provider.tsx | Adds aria-label to theme toggle button |
| src/bricks/button/button.tsx | Adds aria-label prop support and default labels for AcceptButton, CancelButton, and Tag buttons |
| src/app.tsx | Adds skip-to-content link and main landmark with visually hidden h1 |
| package.json | Updates scripts with NODE_OPTIONS for legacy OpenSSL support and localstorage configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| </main> | ||
| <MainFooter /> |
There was a problem hiding this comment.
The closing </main> and <MainFooter /> tags have inconsistent indentation. They should be aligned at the same level as the opening <main> tag for better code readability.
| </main> | |
| <MainFooter /> | |
| </main> | |
| <MainFooter /> |
| "start": "NODE_OPTIONS=\"--openssl-legacy-provider --localstorage-file=/tmp/strichliste-localstorage.json\" react-scripts start", | ||
| "start:legacy": "react-scripts start", | ||
| "build": "NODE_OPTIONS=\"--openssl-legacy-provider --localstorage-file=/tmp/strichliste-localstorage.json\" react-scripts build", | ||
| "build:legacy": "react-scripts build", | ||
| "lint": "eslint --ext .tsx,.ts src/", | ||
| "test": "NODE_OPTIONS=\"--openssl-legacy-provider --localstorage-file=/tmp/strichliste-localstorage.json\" react-scripts test --env=jest-environment-jsdom-sixteen", |
There was a problem hiding this comment.
The --localstorage-file flag appears to be a non-standard Node.js option. NODE_OPTIONS typically accepts flags like --max-old-space-size, --openssl-legacy-provider, etc., but --localstorage-file is not a recognized Node.js flag. This may cause the scripts to fail or the option to be silently ignored. Please verify this is the correct configuration or if this should be handled differently (e.g., as an environment variable or through a different mechanism).
| "start": "NODE_OPTIONS=\"--openssl-legacy-provider --localstorage-file=/tmp/strichliste-localstorage.json\" react-scripts start", | |
| "start:legacy": "react-scripts start", | |
| "build": "NODE_OPTIONS=\"--openssl-legacy-provider --localstorage-file=/tmp/strichliste-localstorage.json\" react-scripts build", | |
| "build:legacy": "react-scripts build", | |
| "lint": "eslint --ext .tsx,.ts src/", | |
| "test": "NODE_OPTIONS=\"--openssl-legacy-provider --localstorage-file=/tmp/strichliste-localstorage.json\" react-scripts test --env=jest-environment-jsdom-sixteen", | |
| "start": "NODE_OPTIONS=\"--openssl-legacy-provider\" react-scripts start", | |
| "start:legacy": "react-scripts start", | |
| "build": "NODE_OPTIONS=\"--openssl-legacy-provider\" react-scripts build", | |
| "build:legacy": "react-scripts build", | |
| "lint": "eslint --ext .tsx,.ts src/", | |
| "test": "NODE_OPTIONS=\"--openssl-legacy-provider\" react-scripts test --env=jest-environment-jsdom-sixteen", |
| <button aria-label="remove"> | ||
| <CancelIcon /> | ||
| </button> | ||
| <button>{children}</button> |
There was a problem hiding this comment.
The second button in the Tag component lacks an accessible name. While it has visible text content ({children}), it should have an explicit aria-label or the button's purpose should be clear from its text content. Consider adding an appropriate aria-label if the children content may not provide sufficient context for screen reader users.
| title={intl.formatMessage({ id: 'ARTICLE_ADD_LINK' })} | ||
| aria-label={intl.formatMessage({ id: 'ARTICLE_ADD_LINK' })} |
There was a problem hiding this comment.
The title and aria-label attributes have inconsistent indentation. The title attribute should be aligned with the other props on the Button component.
| title={intl.formatMessage({ id: 'ARTICLE_ADD_LINK' })} | |
| aria-label={intl.formatMessage({ id: 'ARTICLE_ADD_LINK' })} | |
| title={intl.formatMessage({ id: 'ARTICLE_ADD_LINK' })} | |
| aria-label={intl.formatMessage({ id: 'ARTICLE_ADD_LINK' })} |
| setValue(getValueFromProps()); | ||
| } | ||
| } | ||
| }, [props.value, lastPropValue, getValueFromProps]); |
There was a problem hiding this comment.
The getValueFromProps callback is included in the useEffect dependency array, which will cause this effect to run on every render since the callback is recreated each time props.value changes. This creates a circular dependency: when props.value changes, getValueFromProps is recreated, triggering the effect, which calls getValueFromProps() and updates state, potentially causing unnecessary re-renders. Consider removing getValueFromProps from the dependency array since props.value is already there and directly captures the needed value.
| }, [props.value, lastPropValue, getValueFromProps]); | |
| }, [props.value, lastPropValue]); |
| public getValueFromProps(): number { | ||
| return this.props.value ? this.props.value / 100 : 0; | ||
| } | ||
| const getValueFromProps = React.useCallback(() => (props.value ? props.value / 100 : 0), [props.value]); |
There was a problem hiding this comment.
The getValueFromProps callback using useCallback is unnecessary since it's only used during initialization and in the useEffect. Consider replacing it with a simple inline function or directly computing the value where needed to reduce complexity.
This pull request introduces several improvements focused on accessibility, internationalization, and code modernization. The most significant changes include enhanced accessibility for screen readers, dynamic currency support based on user settings, refactoring of the
CurrencyInputcomponent to use React hooks, and updates to test suites to better reflect state management. Additionally, several scripts and styles have been updated for improved developer experience and UI consistency.