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.

Sign up for GitHub

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

keyboard navigation: feature/focus states #589

Merged
merged 12 commits into from
Oct 29, 2024
Merged

keyboard navigation: feature/focus states #589

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
d95f5f1
prototype focus styling for header elements
interim17 Oct 10, 2024
46b0ffd
use custom dropdown wrapper to keep stylesheets dry
interim17 Oct 10, 2024
6d3a4ff
focus style on AICS logo button
interim17 Oct 10, 2024
0db3d98
Merge branch 'main' of https://github.com/simularium/simularium-websi…
interim17 Oct 14, 2024
13fb445
updated focus and hover stylings for header buttons and dropdown items
interim17 Oct 16, 2024
23f10a4
remove commented code
interim17 Oct 16, 2024
a5007c3
remove unused imports
interim17 Oct 21, 2024
ee6cbe4
Merge branch 'main' of https://github.com/simularium/simularium-websi…
interim17 Oct 22, 2024
2c2ba63
Merge branch 'main' of https://github.com/simularium/simularium-websi…
interim17 Oct 22, 2024
5b67b81
remove narrow property on CustomDropdown to style with fit-content
interim17 Oct 29, 2024
6284eee
use focus-visible selector for keyboard navigation styling in stead o…
interim17 Oct 29, 2024
c2ac2a0
unused import
interim17 Oct 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions src/components/CustomDropdown/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import React, { ReactNode } from "react";
import classNames from "classnames";
import { Dropdown, DropDownProps, MenuProps } from "antd";
import { ButtonClass } from "../../constants/interfaces";
import NavButton from "../NavButton";

import styles from "./style.css";

interface CustomDropdownProps {
items: MenuProps["items"];
titleText: string;
icon: ReactNode;
buttonType: ButtonClass;
placement?: DropDownProps["placement"];
disabled?: boolean;
narrow?: boolean;
}

const CustomDropdown: React.FC<CustomDropdownProps> = ({
items,
titleText,
icon,
buttonType,
placement,
disabled,
narrow,
}) => {
const menuClassNames = narrow
? classNames(styles.menu, styles.narrow)
: styles.menu;
return (
<Dropdown
menu={{ items, theme: "dark", className: menuClassNames }}
placement={placement}
disabled={disabled}
>
<NavButton
titleText={titleText}
icon={icon}
buttonType={buttonType}
/>
</Dropdown>
);
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like our CustomModal and NavButton this wrapper is mostly to DRY out the code, and will be a vehicle for code for things that antd doesn't provide but can be generalized to all dropdowns.


export default CustomDropdown;
48 changes: 48 additions & 0 deletions src/components/CustomDropdown/style.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
.menu {
width: var(--dropdown-menu-width);
background-color: var(--dark-theme-dropdown-menu-bg);
border-radius: 3px;
padding: 8px;
}

.menu.narrow {
width: calc(var(--dropdown-menu-width) - 50px);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: This seems a little magic number-y, would it make more sense to either define a narrow width variable or make the width set by fit-content or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Much better, thank you

}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file replaces the now deleted stylesheets for HelpMenu and LoadFileMenu.

This was the only style difference between the two to manage for now.


.menu svg {
fill: var(--dark-theme-menu-text-color);
}

.menu :global(.ant-dropdown-menu-item),
.menu :global(.ant-dropdown-menu-submenu) {
border: 2px solid var(--dark-theme-dropdown-menu-item-bg);
border-radius: 4px;
height: 32px;
}

.menu :global(.ant-dropdown-menu-item) :global(.ant-btn) {
color: var(--dark-theme-dropdown-menu-item-color);
padding: 0px;
}

.menu :global(.ant-dropdown-menu-item):hover,
.menu :global(.ant-dropdown-menu-submenu):hover {
background-color: var(--dark-theme-dropdown-menu-item-hover-bg);
}

.menu :global(.ant-dropdown-menu-item):hover *,
.menu :global(.ant-dropdown-menu-submenu):hover * {
color: var(--dark-theme-dropdown-menu-item-hover-color);
fill: var(--dark-theme-dropdown-menu-item-hover-color);
}

.menu :global(.ant-dropdown-menu-item):focus-within,
.menu :global(.ant-dropdown-menu-submenu):focus-within {
z-index: 1000;
background-color: var(--dark-theme-dropdown-menu-item-focus-bg);
outline: 1px solid var(--dark-theme-dropdown-menu-item-focus-outline);
}

.menu :global(.ant-dropdown-menu-item) *:focus {
color: var(--dark-theme-dropdown-menu-item-focus-color);
}
30 changes: 14 additions & 16 deletions src/components/HelpMenu/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import * as React from "react";
import { useLocation, Link } from "react-router-dom";
import { Dropdown, MenuProps } from "antd";
import { MenuProps } from "antd";

import { TUTORIAL_PATHNAME } from "../../routes";
import {
Expand All @@ -13,8 +13,7 @@ import { ButtonClass } from "../../constants/interfaces";
import { DownArrow } from "../Icons";
import VersionModal from "../VersionModal";
import NavButton from "../NavButton";

import styles from "./style.css";
import CustomDropdown from "../CustomDropdown";

const HelpMenu = (): JSX.Element => {
const [modalVisible, setModalVisible] = React.useState(false);
Expand Down Expand Up @@ -57,7 +56,6 @@ const HelpMenu = (): JSX.Element => {
{
key: "submit-issue",
label: "Submit issue",
popupClassName: styles.submenu,
popupOffset: [-0.45, -4],
children: [
{
Expand Down Expand Up @@ -89,26 +87,26 @@ const HelpMenu = (): JSX.Element => {
{
key: "version",
label: (
<a
onClick={() => {
<NavButton
titleText={"Version info"}
clickHandler={() => {
setModalVisible(!modalVisible);
}}
>
Version info
</a>
buttonType={ButtonClass.DropdownItem}
/>
),
},
];

return (
<>
<Dropdown menu={{ items, theme: "dark", className: styles.menu }}>
<NavButton
titleText={"Help "}
icon={DownArrow}
buttonType={ButtonClass.Secondary}
/>
</Dropdown>
<CustomDropdown
items={items}
titleText={"Help "}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should CustomDropdown/NavButton just add spacing between the titleText and the icon?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why don't we just remove that silly space instead.

icon={DownArrow}
buttonType={ButtonClass.Secondary}
narrow={true}
/>
{modalVisible && <VersionModal setModalVisible={setModalVisible} />}
</>
);
Expand Down
19 changes: 0 additions & 19 deletions src/components/HelpMenu/style.css
View file
Open in desktop

This file was deleted.

30 changes: 13 additions & 17 deletions src/components/LoadFileMenu/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import React, { useState } from "react";
import { Link, useLocation, useHistory } from "react-router-dom";
import { ActionCreator } from "redux";
import { Dropdown, Button, MenuProps } from "antd";
import { MenuProps } from "antd";

import TRAJECTORIES from "../../constants/networked-trajectories";
import { URL_PARAM_KEY_FILE_NAME } from "../../constants";
Expand All @@ -21,8 +21,7 @@ import { VIEWER_PATHNAME } from "../../routes";
import { DownArrow } from "../Icons";
import FileUploadModal from "../FileUploadModal";
import NavButton from "../NavButton";

import styles from "./style.css";
import CustomDropdown from "../CustomDropdown";

interface LoadFileMenuProps {
isBuffering: boolean;
Expand Down Expand Up @@ -73,7 +72,6 @@ const LoadFileMenu = ({
{
key: "from-examples",
label: "Example models",
popupClassName: styles.submenu,
popupOffset: [-0.45, -4],
children: TRAJECTORIES.map((trajectory) => ({
key: trajectory.id,
Expand All @@ -94,9 +92,11 @@ const LoadFileMenu = ({
{
key: "file-upload",
label: (
<Button type="ghost" onClick={showModal}>
Simularium file
</Button>
<NavButton
titleText={"Simularium file"}
clickHandler={showModal}
buttonType={ButtonClass.DropdownItem}
/>
),
},
{
Expand All @@ -117,18 +117,14 @@ const LoadFileMenu = ({

return (
<>
<Dropdown
menu={{ items, theme: "dark", className: styles.menu }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we lose the dark theming here on the dropdown?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No we don't, although we are overriding most of it anyway.

<CustomDropdown
items={items}
titleText={"Load model"}
icon={DownArrow}
buttonType={ButtonClass.Primary}
placement="bottomRight"
disabled={isDisabled}
>
<NavButton
titleText={"Load model"}
icon={DownArrow}
buttonType={ButtonClass.Primary}
isDisabled={isDisabled}
/>
</Dropdown>
Comment on lines -120 to -131
Copy link
Contributor

Choose a reason for hiding this comment

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

Really like that the NavButton gets collapsed into Dropdown in all the instances here :')

/>
{/*
Conditionally rendering the modal this way instead of as a `visible` prop
forces it to re-render every time it is opened, resetting the form inside.
Expand Down
37 changes: 0 additions & 37 deletions src/components/LoadFileMenu/style.css
View file
Open in desktop

This file was deleted.

37 changes: 30 additions & 7 deletions src/components/NavButton/style.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,18 @@
background-color: var(--dark-theme-action-button-bg);
border: none;
color: var(--dark-theme-action-button-color);
padding: 0px;
padding: 4px 8px;
}

.nav-button.action-button:global(.ant-btn:hover),
.nav-button.action-button:global(.ant-btn:focus) {
.nav-button.action-button:global(.ant-btn:hover) {
color: var(--dark-theme-action-button-hover-color);
}

.nav-button.action-button:global(.ant-btn:focus) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider changing the focus selectors to focus-visible, which will make it only respond to keyboard focusing (and not if the user last clicked on the button)

https://developer.mozilla.org/en-US/docs/Web/CSS/:focus-visible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI: I updated the lines relevant to this PR , but not every place that :focus exists in the code base although I have that in mind now for this ongoing work.

color: var(--dark-theme-action-button-focus-color);
outline: 1px solid var(--dark-theme-primary-button-focus-outline);
}

.nav-button.action-button:global(.ant-btn).disabled {
color: var(--dark-theme-action-button-disabled-color);
cursor: not-allowed;
Expand All @@ -26,13 +30,20 @@
color: var(--dark-theme-primary-button-color);
}

.nav-button.primary-button:global(.ant-btn:hover),
.nav-button.primary-button:global(.ant-btn:focus) {
.nav-button.primary-button:global(.ant-btn:hover) {
color: var(--dark-theme-primary-button-hover-color);
background-color: var(--dark-theme-primary-button-hover-bg);
border-color: var(--dark-theme-primary-button-hover-border);
}

.nav-button.primary-button:global(.ant-btn:focus) {
color: var(--dark-theme-primary-button-focus-color);
background-color: var(--dark-theme-primary-button-focus-bg);
border-color: var(--dark-theme-primary-button-focus-border);
outline: 1px solid var(--dark-theme-primary-button-focus-outline);
outline-offset: 2px;
}

.nav-button.primary-button:global(.ant-btn).disabled {
color: var(--dark-theme-primary-button-disabled-color);
background-color: var(--dark-theme-primary-button-disabled-bg);
Expand All @@ -47,16 +58,28 @@
color: var(--dark-theme-secondary-button-color);
}

.nav-button.secondary-button:global(.ant-btn:hover),
.nav-button.secondary-button:global(.ant-btn:focus) {
.nav-button.secondary-button:global(.ant-btn:hover) {
color: var(--dark-theme-secondary-button-hover-color);
background-color: var(--dark-theme-secondary-button-hover-bg);
border-color: var(--dark-theme-secondary-button-hover-border);
}

.nav-button.secondary-button:global(.ant-btn:focus) {
color: var(--dark-theme-secondary-button-focus-color);
background-color: var(--dark-theme-secondary-button-focus-bg);
border-color: var(--dark-theme-secondary-button-focus-border);
outline: 1px solid var(--dark-theme-secondary-button-focus-outline);
outline-offset: 2px;
}

.nav-button.secondary-button:global(.ant-btn).disabled {
color: var(--dark-theme-secondary-button-disabled-color);
background-color: var(--dark-theme-secondary-button-disabled-bg);
border-color: var(--dark-theme-secondary-button-disabled-border);
cursor: not-allowed;
}

.nav-button.dropdown-item-button:global(.ant-btn) {
background-color: inherit;
border: inherit;
Comment on lines +82 to +84
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In some cases we were using <a> to open modals but for accessibility those should be a buttons, even if its easier to style when they are all the same element type. I also felt the styling for all the differenet elements that can be dropdown menu items should be in the CustomDropdown stylesheet.

Using <NavButton />, making the new DropdownItem button class, and using these two inherit rules seemed to be the most concise way to manage selector specificity and keep the styling where I wanted it. Open to suggestions.

}
1 change: 1 addition & 0 deletions src/constants/interfaces.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ export enum ButtonClass {
Primary = "primary-button",
Secondary = "secondary-button",
Action = "action-button",
DropdownItem = "dropdown-item-button",
}

export enum IconGlyphs {
Expand Down
9 changes: 9 additions & 0 deletions src/containers/AppHeader/style.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,15 @@
margin-right: 2em;
}

.left-links img {
padding: 6px;
border-radius: 6px;
}

.left-links a:focus img {
outline: 1px solid var(--dark-theme-primary-button-focus-outline);
}

.home {
margin-left: 2px;
margin-right: 20px;
Expand Down
Loading
Loading