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

Fix iseec 2378 UI shell accessibility #49

Merged
merged 14 commits into from
Jan 26, 2022

Conversation

MthdeSouza
Copy link
Contributor

Context

Jira Issue:
ISEEC-2378

Build Number:

1.4.1-beta.5

Checklist:

  • Has been verified in an integrated environment
  • Has relevant unit and integration tests passing
  • Has no linting, test console, or browser console errors (best effort)
  • Has JSDoc comment blocks for complex code

PR Review Guidance

Additional Info

@MthdeSouza MthdeSouza requested review from Okisa, BenjaminRuby, LucasGrimauth and marcusdroy and removed request for BenjaminRuby December 10, 2021 17:43
timrbula
timrbula previously approved these changes Jan 18, 2022
@timrbula timrbula removed the request for review from marcusdroy January 18, 2022 20:34
@timrbula
Copy link
Member

@MthdeSouza Added a few more a11y things for the header (and fixed some tests, cleaned up some code)

@@ -49,14 +49,14 @@
},
"dependencies": {
"@carbon/elements": "^10.21.0",
"@carbon/themes": "^10.21.0",
Copy link
Member

Choose a reason for hiding this comment

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

Removed redundant dependency

"@stomp/stompjs": "^5.4.2",
"@tippyjs/react": "^4.0.2",
"carbon-icons": "^7.0.7",
"classnames": "^2.2.5",
"date-fns": "^1.30.1",
"dompurify": "^2.0.6",
"downshift": "^5.2.1",
"invariant": "^2.2.3",
"focus-trap-react": "^8.9.1",
Copy link
Member

Choose a reason for hiding this comment

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

Remove dependencies that aren't used and migrated to use of a better focus trap library for focus management from the UIShell

@@ -50,7 +50,7 @@ test('select and remove items', async () => {
expect(queryByLabelText(/Clear filter cat/i)).toBeInTheDocument();
});

const clearButton = getByRole('button', { name: 'Clear Selection' });
const clearButton = getByRole('button', { name: 'Clear selected item' });
Copy link
Member

Choose a reason for hiding this comment

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

Fix test

@@ -4,8 +4,8 @@ import Error403 from './Error403';

test('render Error403 with defaults', async () => {
const { getByText } = render(<Error403 />);
expect(getByText('403 - Access Forbidden')).toBeInTheDocument();
expect(getByText('You’ve found yourself in deep water.')).toBeInTheDocument();
expect(getByText('403 Access Forbidden')).toBeInTheDocument();
Copy link
Member

Choose a reason for hiding this comment

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

Fix test

@@ -5,15 +5,15 @@ import Error404 from './Error404';

test('render Error404 with defaults', async () => {
const { getByText } = render(<Error404 />);
expect(getByText('404 - Page Not Found')).toBeInTheDocument();
expect(getByText('Crikey. Something seems to have swam off with this page.')).toBeInTheDocument();
expect(getByText('404 Page Not Found')).toBeInTheDocument();
Copy link
Member

Choose a reason for hiding this comment

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

Fix test

@@ -8,15 +8,14 @@ const { prefix } = settings;
const HeaderMenuBmrg = React.forwardRef((props, ref) => {
const { isOpen, ...other } = props;
return (
<div
<button
Copy link
Member

Choose a reason for hiding this comment

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

Made this a proper button and fixed some styling for it to match better w/ the rest of the header

))}
</ul>
</div>
<FocusTrap active focusTrapOptions={{ allowOutsideClick: true }}>
Copy link
Member

Choose a reason for hiding this comment

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

Focus trap here to keep the tabbing inside the menu while it is open. ESC closes it.

@@ -12,9 +11,8 @@ import {
NotificationNew24,
} from '@carbon/icons-react';
import { settings } from 'carbon-components';
import FocusTrap from 'focus-trap-react';
Copy link
Member

Choose a reason for hiding this comment

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

I made a few changes here to manage:

  1. Focus traps within the different menus and the sidenav
  2. Handling the "ESC" key to exit out of the sidenav and menus and returning focus to the button associated w/ the menu.
  3. Tab to leave the side panel (not sure if this is the best approach but its not used afaik at the moment)
  4. Removed dead code.

@timrbula timrbula merged commit d6007a9 into main Jan 26, 2022
@timrbula timrbula deleted the fix-ISEEC-2378-ui-shell-accessibility branch March 22, 2022 00:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants