fix: Stacked notifications in compact mode and without content padding #775

jperals · 2023-02-17T16:03:45Z

Description

This PR improves the Flashbar's support for compact mode and the usage of disableContentPaddings. It does basically two things:

Adjusts the position of the notification bar to make sure it never overlaps notification text, especially in compact mode
It adds some spacing between the notifications and the main content when using disableContentPaddings and the notification bar is shown (to make sure it does not overlap the main content)

How has this been tested?

A lot of manual checking on the new page app-layout/with-stacked-notifications-and-table. Visual regression tests on this page can be done after this PR is merged. Existing visual regression tests for Flashbar are expected to fail because of small changes in the notification bar position and also because of changes in the component's bottom margin which change the screenshot area.

To manually test the changes, I suggest to visit the page app-layout/with-stacked-notifications-and-table in the deployment below and verify the following, for each combination of compact mode vs comfortable mode, and Visual Refresh vs Classic:

Notification bar should not overlap notification text
Notification bar should not overlap content below, also when disabling content paddings and also when expanded vs collapsed
When both notifications and table header are sticky, the table header should stick correctly to the Flashbar, without leaving any extra space between the two. (Note: in this case the notification bar does overlap the content; this is expected)

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-02-17T17:01:50Z

Codecov Report

Base: 93.30% // Head: 93.30% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (8f8eb5b) compared to base (896472f).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #775   +/-   ##
=======================================
  Coverage   93.30%   93.30%           
=======================================
  Files         584      584           
  Lines       17123    17129    +6     
  Branches     4722     4728    +6     
=======================================
+ Hits        15976    15982    +6     
  Misses       1067     1067           
  Partials       80       80

Impacted Files	Coverage Δ
src/app-layout/index.tsx	`95.06% <ø> (ø)`
src/app-layout/visual-refresh/notifications.tsx	`100.00% <ø> (ø)`
src/app-layout/notifications/index.tsx	`100.00% <100.00%> (ø)`
src/flashbar/collapsible-flashbar.tsx	`97.95% <100.00%> (+0.05%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

…BottomSpacing to work

jperals · 2023-02-22T12:48:47Z

pages/app-layout/with-stacked-notifications-and-table.page.tsx

@@ -0,0 +1,155 @@
+// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.


The recently added page app-layout/with-stacked-notifications-and-no-paddings was deleted in favor of this one, which is more complete and makes it redundant. No tests had been written for the former yet.

jperals · 2023-02-22T12:52:26Z

src/flashbar/collapsible-flashbar.tsx

@@ -112,7 +113,7 @@ export default function CollapsibleFlashbar({ items, ...restProps }: FlashbarPro
          const windowHeight = window.innerHeight;
          // Take the parent region into account if using the App Layout, because it might have additional margins.
          // Otherwise we use the Flashbar component for this calculation.
-          const outerElement = flashbar.parentElement?.parentElement || flashbar;
+          const outerElement = findUpUntil(flashbar, element => element.getAttribute('role') === 'region') || flashbar;


Made this more robust as I realized it was not working well on Visual Refresh (it was grabbing one level further up to the correct one). This is because the DOM structure rendered by the app layout can be different on Visual Refresh than Classic —and actually also when notifications are stick vs when they aren't.

jperals · 2023-02-23T09:03:06Z

src/flashbar/collapsible.scss

@@ -208,6 +278,8 @@ the grid layout will be:
    flex-grow: 1;
    background: none;
    border: 0 none;
+    padding-top: 0;


These two lines were moved from lines 140-141 because it only needs to affect .button and not .status > .header nor .status > .item-count. See #772 for more context on what these two lines are for.

…t padding (#775)" This reverts commit d5de598.

#775)

jperals force-pushed the fix/flashbar-compact-mode branch from 0f27dd9 to b1783ba Compare February 17, 2023 16:12

jperals deployed to preview February 17, 2023 17:00 — with GitHub Actions View deployment

jperals deployed to preview February 20, 2023 09:11 — with GitHub Actions View deployment

jperals force-pushed the fix/flashbar-compact-mode branch from 2ee493c to 2b7f98a Compare February 21, 2023 17:44

jperals deployed to preview February 21, 2023 18:07 — with GitHub Actions View deployment

jperals added 22 commits February 22, 2023 13:08

fix: Stacked notifications in compact mode and without content padding

1a629f5

Add split panel to test page

982982e

Make notifications container query more robust

3d2e325

Fix class being applied when it shouldn't

93aa17a

Fine tune notification bar position

3df1ecb

Fine tune test page

3231c99

Bring Flashbar on top of split panel

68b58dd

Fine tune test page

fab3b83

Fine tune test page

e815bec

Use updateBottomSpacing again

b217448

Adjust bottom margin for each case

ca342d3

Restore flashbar-sticky-bottom-margin custom prop required for update…

f5fac25

…BottomSpacing to work

Add test ids to test page

b775c39

Refactor

5e0511f

Fix region element retrieval in updateBottomSpacing

a2bd98c

Add first-level title to test page

2ee458f

Don't use extra margins in Visual Refresh

8a44970

Add comment

7d204b2

Fix Visual Refresh

0167bad

Refactor

59b4866

Fine tune vertical spacings

aac601e

Minor refactor

c7b75bd

jperals force-pushed the fix/flashbar-compact-mode branch from 576023e to c7b75bd Compare February 22, 2023 12:08

jperals deployed to preview February 22, 2023 12:25 — with GitHub Actions View deployment

jperals marked this pull request as ready for review February 22, 2023 12:37

jperals requested a review from a team as a code owner February 22, 2023 12:37

jperals requested review from connorlanigan and removed request for a team February 22, 2023 12:37

jperals commented Feb 22, 2023

View reviewed changes

jperals added 2 commits February 22, 2023 14:06

Remove redundant CSS rules

b03333f

Fix selector

ee678c7

jperals deployed to preview February 22, 2023 13:19 — with GitHub Actions View deployment

jperals commented Feb 23, 2023

View reviewed changes

connorlanigan previously approved these changes Feb 23, 2023

View reviewed changes

Merge branch 'main' into fix/flashbar-compact-mode

8f8eb5b

jperals dismissed connorlanigan’s stale review via 8f8eb5b February 23, 2023 11:42

connorlanigan approved these changes Feb 23, 2023

View reviewed changes

jperals temporarily deployed to preview February 23, 2023 11:53 — with GitHub Actions Inactive

jperals deployed to preview February 23, 2023 12:34 — with GitHub Actions View deployment

jperals merged commit d5de598 into main Feb 23, 2023

jperals deleted the fix/flashbar-compact-mode branch February 23, 2023 12:43

jperals mentioned this pull request Feb 23, 2023

fix: notification bar border width in Visual Refresh #784

Merged

jperals added a commit that referenced this pull request Feb 24, 2023

Revert "fix: Stacked notifications in compact mode and without conten…

30a5d5a

…t padding (#775)" This reverts commit d5de598.

jperals mentioned this pull request Feb 27, 2023

fix: Restore Flashbar fixes with workaround for postcss-calc issue #790

Merged

abdhalees pushed a commit that referenced this pull request Feb 27, 2023

fix: Stacked notifications in compact mode and without content padding (

f5fbddb

#775)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Stacked notifications in compact mode and without content padding #775

fix: Stacked notifications in compact mode and without content padding #775

jperals commented Feb 17, 2023 •

edited

Loading

codecov bot commented Feb 17, 2023 •

edited

Loading

jperals Feb 22, 2023

jperals Feb 22, 2023

jperals Feb 23, 2023

		@@ -0,0 +1,155 @@
		// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.

fix: Stacked notifications in compact mode and without content padding #775

fix: Stacked notifications in compact mode and without content padding #775

Conversation

jperals commented Feb 17, 2023 • edited Loading

Description

How has this been tested?

Correctness

Security

Testing

codecov bot commented Feb 17, 2023 • edited Loading

Codecov Report

jperals Feb 22, 2023

Choose a reason for hiding this comment

jperals Feb 22, 2023

Choose a reason for hiding this comment

jperals Feb 23, 2023

Choose a reason for hiding this comment

jperals commented Feb 17, 2023 •

edited

Loading

codecov bot commented Feb 17, 2023 •

edited

Loading