keyboard navigation: feature/focus states #589

interim17 · 2024-10-16T21:58:28Z

Time estimate or Size

medium/small

Problem

Closes #575 and #576, advances #347
Part of keyboard navigation and accessibility work.

Solution

Focus styling for action buttons has been reviewed with Lyndsay but is not reflected in master Figma yet.
Styling for primary/secondary buttons, and dropdown elements is there.
Style guide

Closing #576 because they already work, its clear how to tab through them now that the visual style is obivous.

Disabled buttons are taking focus/styling for now, this can be discussed more in future as we get into navigation behavior.

New feature (non-breaking change which adds functionality)

CustomDropdown

Wrapper for our Dropdowns, unifies the stylesheets, just a slight DRYing now, but will be very useful going forward.

Steps to Verify:

Tab through header and try action buttons (AICS home link, Simularium home link, Download, Share)
Open dropdowns and check on hover and focus styling (focus styling is difficult to maintain sometimes with mouse interactions. Easiest way to see it is the use the buttons that open the submenus.

Applying focus styling to ALL interactive elements is out of scope for this PR (e.g. submenu items in drop-downs)

Screenshots (optional):

… into feature/focus-states

interim17 · 2024-10-16T21:59:22Z

src/styles/ant-vars.less

-//Dropdown
-@dropdown-menu-bg: @dark-four;
-@dropdown-vertical-padding: 5px;
-@dropdown-edge-child-vertical-padding: 4px;
-@dropdown-font-size: @font-size-base;
-@dropdown-line-height: 22px;
-


Continuing to pivot away from .less in anticipation of migrating to antd 5

github-actions · 2024-10-16T21:59:55Z

Coverage report

St.^❔	Category	Percentage	Covered / Total
🟡	Statements	66.27% (-0.46% 🔻)	676/1020
🟡	Branches	65.31% (-1.36% 🔻)	96/147
🔴	Functions	35% (-0.25% 🔻)	91/260
🟡	Lines	64.66% (-0.41% 🔻)	602/931

Test suite run success

121 tests passing in 7 suites.

Report generated by 🧪jest coverage report action from c2ac2a0

interim17 · 2024-10-16T22:06:26Z

src/components/NavButton/style.css

+.nav-button.dropdown-item-button:global(.ant-btn) {
+    background-color: inherit;
+    border: inherit;


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.

… into feature/focus-states

interim17 · 2024-10-28T21:22:51Z

src/components/CustomDropdown/index.tsx

+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>
+    );
+};


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.

interim17 · 2024-10-28T21:23:57Z

src/components/CustomDropdown/style.css

+.menu.narrow {
+    width: calc(var(--dropdown-menu-width) - 50px);
+}


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

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

toloudis

Taking for granted that everything works, I see no obvious code smells. LGTM!

ShrimpCryptid

Left some non-blocking comments but love the accessibility changes.

ShrimpCryptid · 2024-10-29T18:33:51Z

src/components/HelpMenu/index.tsx

-            </Dropdown>
+            <CustomDropdown
+                items={items}
+                titleText={"Help "}


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

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

ShrimpCryptid · 2024-10-29T18:35:08Z

src/components/CustomDropdown/style.css

+}
+
+.menu.narrow {
+    width: calc(var(--dropdown-menu-width) - 50px);


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?

Much better, thank you

ShrimpCryptid · 2024-10-29T18:45:09Z

src/components/LoadFileMenu/index.tsx

-            <Dropdown
-                menu={{ items, theme: "dark", className: styles.menu }}
+            <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>


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

ShrimpCryptid · 2024-10-29T18:46:10Z

src/components/LoadFileMenu/index.tsx

@@ -117,18 +117,14 @@ const LoadFileMenu = ({

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


Do we lose the dark theming here on the dropdown?

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

ShrimpCryptid · 2024-10-29T18:50:32Z

src/components/NavButton/style.css

    color: var(--dark-theme-action-button-hover-color);
 }

+.nav-button.action-button:global(.ant-btn:focus) {


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

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.

…f :focus

interim17 added 6 commits October 10, 2024 10:04

prototype focus styling for header elements

d95f5f1

use custom dropdown wrapper to keep stylesheets dry

46b0ffd

focus style on AICS logo button

6d3a4ff

Merge branch 'main' of https://github.com/simularium/simularium-website…

0db3d98

… into feature/focus-states

updated focus and hover stylings for header buttons and dropdown items

13fb445

remove commented code

23f10a4

interim17 commented Oct 16, 2024

View reviewed changes

interim17 marked this pull request as ready for review October 16, 2024 22:06

interim17 requested a review from a team as a code owner October 16, 2024 22:06

interim17 requested review from toloudis and frasercl and removed request for a team October 16, 2024 22:06

interim17 added 3 commits October 21, 2024 13:10

remove unused imports

a5007c3

Merge branch 'main' of https://github.com/simularium/simularium-website…

ee6cbe4

… into feature/focus-states

Merge branch 'main' of https://github.com/simularium/simularium-website…

2c2ba63

… into feature/focus-states

interim17 changed the title ~~feature/focus states~~ keyboard navigation: feature/focus states Oct 23, 2024

interim17 commented Oct 28, 2024

View reviewed changes

ShrimpCryptid requested review from ShrimpCryptid and removed request for frasercl October 28, 2024 21:35

toloudis approved these changes Oct 28, 2024

View reviewed changes

ShrimpCryptid approved these changes Oct 29, 2024

View reviewed changes

interim17 added 3 commits October 29, 2024 16:35

remove narrow property on CustomDropdown to style with fit-content

5b67b81

use focus-visible selector for keyboard navigation styling in stead o…

6284eee

…f :focus

unused import

c2ac2a0

interim17 merged commit 696f7bd into main Oct 29, 2024
6 checks passed

interim17 deleted the feature/focus-states branch October 29, 2024 23:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

keyboard navigation: feature/focus states #589

keyboard navigation: feature/focus states #589

interim17 commented Oct 16, 2024

interim17 Oct 16, 2024

github-actions bot commented Oct 16, 2024 •

edited

Loading

interim17 Oct 16, 2024

interim17 Oct 28, 2024

interim17 Oct 28, 2024

toloudis left a comment

ShrimpCryptid left a comment

ShrimpCryptid Oct 29, 2024

interim17 Oct 29, 2024

ShrimpCryptid Oct 29, 2024

interim17 Oct 29, 2024

ShrimpCryptid Oct 29, 2024

ShrimpCryptid Oct 29, 2024

interim17 Oct 29, 2024

ShrimpCryptid Oct 29, 2024

interim17 Oct 29, 2024

interim17 Oct 29, 2024

keyboard navigation: feature/focus states #589

keyboard navigation: feature/focus states #589

Conversation

interim17 commented Oct 16, 2024

Time estimate or Size

Problem

Solution

CustomDropdown

Steps to Verify:

Screenshots (optional):

Choose a reason for hiding this comment

github-actions bot commented Oct 16, 2024 • edited Loading

Coverage report

Test suite run success

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toloudis left a comment

Choose a reason for hiding this comment

ShrimpCryptid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 16, 2024 •

edited

Loading