Feat/content examples #6
Conversation
|
♿ Pa11y detected 1 accessibility issue(s) – please review here. |
|
♿ Axe detected 1 accessibility issue(s) – please review here. |
|
♿ Lighthouse detected 8 accessibility issue(s) – please review here. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a great initiative to add example content for all WCAG 2.2 success criteria. My review focuses on fixing a few bugs and improving the overall code quality of the implementation, rather than the intentional accessibility violations in the examples themselves. I've identified some critical accessibility issues in the main index.html file, such as a missing lang attribute and an empty <title>, which should be addressed. Additionally, I've found a typo in a CSS variable, the use of a deprecated API, direct DOM manipulation that goes against React best practices, and several instances of inline styling that could be moved to CSS classes for better maintainability. I've also suggested a fix for a usability issue with button cursors.
| @@ -1,5 +1,5 @@ | |||
| <!doctype html> | |||
| <html lang="en"> | |||
| <html> | |||
There was a problem hiding this comment.
The lang attribute has been removed from the <html> tag. This is a critical accessibility issue as it violates WCAG 3.1.1 (Language of Page). Screen readers and other assistive technologies rely on this attribute to use the correct pronunciation and character sets. Please restore it to ensure the page itself is accessible.1
| <html> | |
| <html lang="en"> |
Style Guide References
Footnotes
-
The resulting application must meet WCAG 2.2 AA standards. ↩
| href="https://ui-components.ubilabs.com/favicon/v2-rc/apple-touch-icon.png" /> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0" /> | ||
| <title>Accessibility Done Wrong</title> | ||
| <title></title> |
There was a problem hiding this comment.
The <title> tag is empty. This violates WCAG 2.4.2 (Page Titled). A descriptive page title is essential for users to understand the page's purpose, especially for screen reader users navigating between tabs or windows. Please provide a meaningful title.1
| <title></title> | |
| <title>Accessibility Done Wrong</title> |
Style Guide References
Footnotes
-
The resulting application must meet WCAG 2.2 AA standards. ↩
| const handleOrientationChange = (event: DeviceOrientationEvent) => { | ||
| if (isFirstEvent) { | ||
| isFirstEvent = false; | ||
| return; | ||
| } | ||
|
|
||
| if (event.beta !== null && motionContentRef.current) { | ||
| motionContentRef.current.innerText = `Content refreshed at ${new Date().toLocaleTimeString()}`; | ||
| } | ||
| }; |
There was a problem hiding this comment.
The DeviceOrientationEvent API is deprecated and not recommended for new projects due to inconsistent browser support and security concerns. For a more modern and supported approach to orientation, consider using the ScreenOrientation API. Given this is for a demo, it might be acceptable, but it's worth noting for future work.
| } | ||
|
|
||
| if (event.beta !== null && motionContentRef.current) { | ||
| motionContentRef.current.innerText = `Content refreshed at ${new Date().toLocaleTimeString()}`; |
There was a problem hiding this comment.
You are directly manipulating the DOM using innerText. In React, it's a best practice to manage content through state to ensure the component's behavior is predictable and easy to debug. This also aligns with the style guide's principle of maintainability.1
You could refactor this by introducing a new state variable:
const [motionContent, setMotionContent] = useState('Tilt your device to refresh the content.');Then update it in your effect:
if (event.beta !== null) {
setMotionContent(`Content refreshed at ${new Date().toLocaleTimeString()}`);
}And finally, render it in your JSX, removing the need for the motionContentRef:
<div ref={motionContentRef}>{motionContent}</div>Style Guide References
Footnotes
-
Code should be easy to update, extend, and debug. ↩
| <p>Turn on the oven.</p> | ||
| </div> | ||
|
|
||
| <div style={{gridRow: '3'}}> |
There was a problem hiding this comment.
Inline styles are used here and in several other places (e.g., lines 605, 608, 723, 727, 822, 2032). For better maintainability, consistency, and separation of concerns, it's recommended to move these styles to your CSS module file (App.module.css) and apply them via class names.1
For example, you could create a class like this in App.module.css:
.meaningfulSequenceOutOfOrder {
grid-row: 3;
}| <div style={{gridRow: '3'}}> | |
| <div className={styles.meaningfulSequenceOutOfOrder}> |
Style Guide References
Footnotes
-
Code should be easy to update, extend, and debug. Following project conventions and this style guide is also a key principle. ↩
| color: var(--ubilabs-white); | ||
| font: var(--ubilabs-default-font); | ||
| font-weight: 500; | ||
| cursor: default; |
There was a problem hiding this comment.
Setting cursor: default; for buttons is not standard practice and harms usability. Users expect a pointer cursor to indicate that an element is clickable. Please change this to cursor: pointer; to improve the user experience.
| cursor: default; | |
| cursor: pointer; |
plumdumpling
force-pushed
the
feat/content-examples
branch
from
f66ccb3 to
3437ebb
Compare
|
♿ Pa11y detected 1 accessibility issue(s) – please review here. |
|
♿ Lighthouse detected 8 accessibility issue(s) – please review here. |
|
♿ Pa11y detected 1 accessibility issue(s) – please review here. |
|
♿ Lighthouse detected 8 accessibility issue(s) – please review here. |
plumdumpling
force-pushed
the
feat/content-examples
branch
from
3437ebb to
d1e4736
Compare
|
♿ Pa11y detected 1 accessibility issue(s) – please review here. |
|
♿ Lighthouse detected 8 accessibility issue(s) – please review here. |
plumdumpling
force-pushed
the
feat/content-examples
branch
from
d1e4736 to
40ae870
Compare
|
♿ Pa11y detected 1 accessibility issue(s) – please review here. |
|
♿ Lighthouse detected 8 accessibility issue(s) – please review here. |
1 participant
add example content for all WCAG 2.2 success criteria