fix: Fix visual bugs in drawer toggle buttons #1343

just-boris · 2023-07-18T14:30:09Z

Description

Fix focus and hover style issues with drawer triggers.

Updated the non-drawer toggle buttons too, to have unified implementation

Related links, issue #, if available: n/a

How has this been tested?

PR build
Visual regression tests in my pipeline

Review checklist

The following items are to be evaluated by the author(s) and the reviewer(s).

Correctness

Changes include appropriate documentation updates.
Changes are backward-compatible if not indicated, see CONTRIBUTING.md.
Changes do not include unsupported browser features, see CONTRIBUTING.md.
Changes were manually tested for accessibility, see accessibility guidelines.

Security

If the code handles URLs: all URLs are validated through the checkSafeUrl function.

Testing

Changes are covered with new/existing unit tests?
Changes are covered with new/existing integration tests?

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

codecov · 2023-07-18T15:12:28Z

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (b80535d) 93.46% compared to head (36bc0cc) 93.46%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1343   +/-   ##
=======================================
  Coverage   93.46%   93.46%           
=======================================
  Files         623      623           
  Lines       16731    16733    +2     
  Branches     5527     5527           
=======================================
+ Hits        15637    15639    +2     
  Misses       1021     1021           
  Partials       73       73

Impacted Files	Coverage Δ
src/app-layout/visual-refresh/drawers.tsx	`84.28% <ø> (ø)`
src/app-layout/drawer/index.tsx	`97.61% <100.00%> (ø)`
src/app-layout/mobile-toolbar/index.tsx	`92.85% <100.00%> (ø)`
src/app-layout/toggles/index.tsx	`100.00% <100.00%> (ø)`
src/app-layout/visual-refresh/trigger-button.tsx	`100.00% <100.00%> (ø)`
src/test-utils/dom/app-layout/index.ts	`100.00% <100.00%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

just-boris · 2023-07-19T14:36:36Z

src/app-layout/__integ__/app-layout-focus-delegation.test.ts

@@ -91,13 +91,14 @@ function setupTest(
          'drawers focus toggles between open and close buttons',
          setupTest(
            async page => {
-              await page.click(wrapper.findDrawersTriggers().get(2).toSelector());
+              const triggerSelector = wrapper.findDrawerTriggerById('pro-help').toSelector();


:nth-child does not work anymore, because every button has a wrapper now.

Since this feature is not in production yet, we can do such breaking changes

just-boris · 2023-07-19T14:40:26Z

src/app-layout/constants.scss

+$_toggle-padding-horizontal: 0.7 * styles.$base-size;
+$_toggle-padding-vertical: awsui.$space-xxs;


Prefix a variable with underscore makes it available only inside the current file

just-boris · 2023-07-19T14:43:45Z

src/app-layout/drawer/styles.scss

+    &.drawer-content-clickable {
+      cursor: pointer;
+      color: awsui.$color-text-interactive-default;


Made some refactoring in this file to more clearly connect styles and their reasons.

Why is there a pointer cursor? Not because the drawer is closed, but because it is clickable

There could be closed non-clickable drawers and the new CSS structure allows that

just-boris · 2023-07-19T14:47:00Z

src/app-layout/drawer/styles.scss

@@ -63,34 +62,26 @@ $drawer-z-index-mobile: 1001;
  }
 }

-.trigger {
-  .drawer-content > .drawer-triggers > & {


Before this PR, app layout used our button component and this line overrode button styles.

Overriding styles is bad even if done for a good reason, it still may cause issues.

In this PR I replaced it with custom styles on top of native <button> tag (see the <ToggleButton> component) which has the correct styles from the start, without overrides

just-boris · 2023-07-19T14:47:38Z

src/app-layout/mobile-toolbar/styles.scss

@@ -12,7 +12,6 @@
 }

 .mobile-bar {
-  /* stylelint-disable-next-line plugin/no-unsupported-browser-features */


Not needed anymore with our current browser support

just-boris · 2023-07-19T14:49:05Z

src/app-layout/toggles/interfaces.ts

@@ -8,6 +9,7 @@ export interface AppLayoutButtonProps {
  ariaExpanded?: boolean;
  iconName?: IconProps.Name;
  iconSvg?: React.ReactNode;
-  onClick: () => void;
+  onClick?: () => void;


Made it optional, because in some cases, the wrapper element is clickable and button does not have an its own listener

just-boris · 2023-07-19T14:50:08Z

style-dictionary/utils/token-names.ts

@@ -331,7 +331,6 @@ export type ColorsTokenName =
  | 'colorForegroundControlDefault'
  | 'colorForegroundControlDisabled'
  | 'colorShadowDefault'
-  | 'colorShadowLayoutToggle'


Replaced this token with colorBorderLayout rendering the same look

jperals · 2023-07-20T08:46:04Z

Before this change the focus rings in Classic didn't look right, they were cut off:

After this change they are shown inside the corresponding toggle button, which gets rid of this particular problem, but there are still a couple things that I think could be improved:

They have some margin to the container horizontally but stick to the border vertically:

Maybe more importantly, they are hard to distinguish over the currently active item:

I see two possible ways to fix this:

Fine tune the dimensions to have some margin both horizontally and vertically, and use the bright variant of the focus ring color for the currently active item
Bring the focus ring back to be outside of the drawer toggle buttons, fixing the cutoff issue. As a quick experiment, by removing overflow: auto from the drawer-content they look like this:

just-boris · 2023-07-20T09:17:51Z

I knew about this issue and I will fix it in the next iteration. The current state is not a regression comparing to before

just-boris · 2023-07-20T09:19:56Z

The plan is to reuse this treatment from calendar

just-boris force-pushed the fix-classic-drawers branch 2 times, most recently from ac28d2a to 1f423ae Compare July 18, 2023 14:57

just-boris deployed to preview July 18, 2023 15:11 — with GitHub Actions View deployment

just-boris force-pushed the fix-classic-drawers branch from 1f423ae to 63d23db Compare July 18, 2023 15:32

just-boris deployed to preview July 18, 2023 15:49 — with GitHub Actions View deployment

just-boris force-pushed the fix-classic-drawers branch from 63d23db to 2f4b458 Compare July 18, 2023 16:04

just-boris deployed to preview July 18, 2023 16:21 — with GitHub Actions View deployment

just-boris force-pushed the fix-classic-drawers branch from 2f4b458 to 503f7f5 Compare July 18, 2023 17:38

just-boris deployed to preview July 18, 2023 17:51 — with GitHub Actions View deployment

just-boris force-pushed the fix-classic-drawers branch 2 times, most recently from 21ff56e to e526398 Compare July 18, 2023 20:20

just-boris deployed to preview July 18, 2023 20:38 — with GitHub Actions View deployment

just-boris force-pushed the fix-classic-drawers branch from e526398 to fd2044e Compare July 19, 2023 11:49

just-boris deployed to preview July 19, 2023 12:03 — with GitHub Actions View deployment

just-boris marked this pull request as ready for review July 19, 2023 14:35

just-boris requested a review from a team as a code owner July 19, 2023 14:35

just-boris requested review from jperals and removed request for a team July 19, 2023 14:35

just-boris commented Jul 19, 2023

View reviewed changes

fix: Fix visual bugs in drawer toggle buttons

36bc0cc

just-boris force-pushed the fix-classic-drawers branch from fd2044e to 36bc0cc Compare July 19, 2023 15:05

just-boris deployed to preview July 19, 2023 15:18 — with GitHub Actions View deployment

jperals approved these changes Jul 20, 2023

View reviewed changes

just-boris merged commit 5881b58 into main Jul 20, 2023
23 checks passed

just-boris deleted the fix-classic-drawers branch July 20, 2023 09:33

just-boris mentioned this pull request Jul 20, 2023

chore: Fix focus outline on app layout triggers #1353

Merged

just-boris mentioned this pull request Aug 8, 2023

chore: Drawer badge, mobile and overflow #1416

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Fix visual bugs in drawer toggle buttons #1343

fix: Fix visual bugs in drawer toggle buttons #1343

just-boris commented Jul 18, 2023

codecov bot commented Jul 18, 2023 •

edited

Loading

just-boris Jul 19, 2023

just-boris Jul 19, 2023

just-boris Jul 19, 2023

just-boris Jul 19, 2023

just-boris Jul 19, 2023

just-boris Jul 19, 2023

just-boris Jul 19, 2023

just-boris Jul 19, 2023

jperals commented Jul 20, 2023

just-boris commented Jul 20, 2023

just-boris commented Jul 20, 2023

		$_toggle-padding-horizontal: 0.7 * styles.$base-size;
		$_toggle-padding-vertical: awsui.$space-xxs;

fix: Fix visual bugs in drawer toggle buttons #1343

fix: Fix visual bugs in drawer toggle buttons #1343

Conversation

just-boris commented Jul 18, 2023

Description

How has this been tested?

Correctness

Security

Testing

codecov bot commented Jul 18, 2023 • edited Loading

Codecov Report

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

jperals commented Jul 20, 2023

just-boris commented Jul 20, 2023

just-boris commented Jul 20, 2023

codecov bot commented Jul 18, 2023 •

edited

Loading