fix: Empty notifications height #2856

just-boris · 2024-10-11T15:58:52Z

Description

Widget friendly version of #2810. Thanks @georgylobko for the original code.

See inline comments for more details

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

How has this been tested?

Locally
Updated integration test
New screenshot test CR-153803954

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.

just-boris · 2024-10-11T15:59:32Z

pages/app-layout/utils/content-blocks.tsx

-      ]}
-    />
-  );
+  return <Flashbar items={notifications} />;


Updated demo implementation to represent real-world use-cases more closely

just-boris · 2024-10-11T16:00:33Z

src/app-layout/__integ__/awsui-applayout.test.ts

@@ -167,11 +167,18 @@ describe.each(['classic', 'refresh', 'refresh-toolbar'] as Theme[])('%s', theme
  test(
    'does not render notifications slot when it is empty',


We already had a test which was supposed to catch the issue, but it was asserting size of the wrong element. Refactored the test. It is now more strict and may require an update even on unrelated design changes, but this is still better than silently passing

just-boris · 2024-10-11T16:01:29Z

src/app-layout/visual-refresh-toolbar/notifications/index.tsx

@@ -23,8 +24,9 @@ export function AppLayoutNotificationsImplementation({
  children,
 }: AppLayoutNotificationsImplementationProps) {
  const { ariaLabels, stickyNotifications, setNotificationsHeight, verticalOffsets } = appLayoutInternals;
-  const ref = useRef<HTMLElement>(null);
-  useResizeObserver(ref, entry => setNotificationsHeight(entry.borderBoxHeight));
+  const [hasNotificationsContent, contentRef] = useContainerQuery(rect => rect.borderBoxHeight > 0);


an extra resize observer may seem like a waste, but it allows us to resolve this rendering logic without talking outside this component, which is useful in the widget environment (compatibility with existing code)

codecov · 2024-10-11T16:12:56Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.19%. Comparing base (09a1b44) to head (c07c3ef).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2856      +/-   ##
==========================================
- Coverage   96.20%   96.19%   -0.01%     
==========================================
  Files         761      761              
  Lines       21436    21438       +2     
  Branches     6939     7327     +388     
==========================================
  Hits        20622    20622              
+ Misses        806      763      -43     
- Partials        8       53      +45

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

georgylobko · 2024-10-11T16:17:28Z

pages/app-layout/utils/content-blocks.tsx

-    return null;
+  const notifications: Array<FlashbarProps.MessageDefinition> = [];
+  if (visible) {
+    notifications.push({


Why do you push an element to the array on the rendering condition? Why not directly provide an array with a single item based on visible? At least it would be more readable

Do you want something like this

return <Flashbar items={visible ? [demoNotification] : []}>

just-boris · 2024-10-14T10:57:04Z

src/app-layout/__integ__/awsui-applayout.test.ts

@@ -167,11 +167,19 @@ describe.each(['classic', 'refresh', 'refresh-toolbar'] as Theme[])('%s', theme
  test(
    'does not render notifications slot when it is empty',
    setupTest({ pageName: 'with-notifications' }, async page => {
-      const { height: originalHeight } = await page.getBoundingBox(wrapper.findNotifications().toSelector());
-      expect(originalHeight).toBeGreaterThan(0);
+      const { height: headerHeight } = await page.getBoundingBox('#h');


This height is different between CI and my local. I got it working when reading this value dynamically

Could you elaborate what is "dynamic" in this code? And why the height is different in different environments?

The header height depends on system font rendering which is not deterministic and varies between Mac/Linux.

Tried to make it more deterministic: 14de51b

It broke other tests, so I abandoned this path

georgylobko · 2024-10-14T11:24:24Z

src/app-layout/__integ__/awsui-applayout.test.ts

@@ -167,11 +167,19 @@ describe.each(['classic', 'refresh', 'refresh-toolbar'] as Theme[])('%s', theme
  test(
    'does not render notifications slot when it is empty',
    setupTest({ pageName: 'with-notifications' }, async page => {
-      const { height: originalHeight } = await page.getBoundingBox(wrapper.findNotifications().toSelector());
-      expect(originalHeight).toBeGreaterThan(0);
+      const { height: headerHeight } = await page.getBoundingBox('#h');


Could you elaborate what is "dynamic" in this code? And why the height is different in different environments?

georgylobko · 2024-10-14T11:33:20Z

src/app-layout/__integ__/awsui-applayout.test.ts

+        refresh: headerHeight + 12,
+        'refresh-toolbar': headerHeight + 54,
+      }[theme];
+      expect(contentTop).toEqual(expectedOffset);


Isn't it be easier to only check notification container height and make sure it equals 0?

This is how original test was structured and it still allowed this regression to happen.

The new approach makes sure this test fails every time there is an extended top offset of the main content, regardless how it is implemented internally

just-boris commented Oct 11, 2024

View reviewed changes

just-boris changed the title ~~Fix/empty notifications height 2~~ fix: Empty notifications height Oct 11, 2024

just-boris force-pushed the fix/empty-notifications-height-2 branch from 361636e to 936684e Compare October 11, 2024 16:04

just-boris deployed to dev-pages October 11, 2024 16:12 — with GitHub Actions View deployment

just-boris requested a review from georgylobko October 11, 2024 16:16

just-boris marked this pull request as ready for review October 11, 2024 16:16

just-boris requested a review from a team as a code owner October 11, 2024 16:16

just-boris removed the request for review from a team October 11, 2024 16:16

georgylobko reviewed Oct 11, 2024

View reviewed changes

just-boris requested a review from georgylobko October 11, 2024 16:24

georgylobko previously approved these changes Oct 11, 2024

View reviewed changes

just-boris deployed to dev-pages October 11, 2024 16:32 — with GitHub Actions View deployment

just-boris dismissed georgylobko’s stale review via 14de51b October 12, 2024 06:55

just-boris deployed to dev-pages October 12, 2024 07:03 — with GitHub Actions View deployment

just-boris force-pushed the fix/empty-notifications-height-2 branch from 14de51b to 82e17e3 Compare October 12, 2024 07:39

just-boris deployed to dev-pages October 12, 2024 07:47 — with GitHub Actions View deployment

just-boris force-pushed the fix/empty-notifications-height-2 branch from 82e17e3 to 92c4e40 Compare October 14, 2024 10:33

just-boris deployed to dev-pages October 14, 2024 10:41 — with GitHub Actions View deployment

just-boris requested a review from georgylobko October 14, 2024 10:55

just-boris commented Oct 14, 2024

View reviewed changes

just-boris enabled auto-merge October 14, 2024 11:11

georgylobko reviewed Oct 14, 2024

View reviewed changes

georgylobko and others added 7 commits October 14, 2024 14:42

fix: Empty notifications height

adad2a0

fix: Calculate notifications' height based on child element

5f13f7c

fix: Take tokens.spaceXs into sticky top offset calculation

2ad93d1

refactor test implementation

842daf9

update flashbar demo

b96134b

define header height

787749a

refactor integration test to avoid hard-coded values

c07c3ef

just-boris force-pushed the fix/empty-notifications-height-2 branch from 92c4e40 to c07c3ef Compare October 14, 2024 12:43

just-boris deployed to dev-pages October 14, 2024 12:51 — with GitHub Actions View deployment

georgylobko approved these changes Oct 14, 2024

View reviewed changes

just-boris added this pull request to the merge queue Oct 14, 2024

Merged via the queue into main with commit 25f4435 Oct 14, 2024
34 checks passed

just-boris deleted the fix/empty-notifications-height-2 branch October 14, 2024 13:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Empty notifications height #2856

fix: Empty notifications height #2856

just-boris commented Oct 11, 2024 •

edited

Loading

just-boris Oct 11, 2024

just-boris Oct 11, 2024

just-boris Oct 11, 2024

codecov bot commented Oct 11, 2024 •

edited

Loading

georgylobko Oct 11, 2024

just-boris Oct 11, 2024

georgylobko Oct 11, 2024

just-boris Oct 11, 2024

just-boris Oct 14, 2024

georgylobko Oct 14, 2024

just-boris Oct 14, 2024

georgylobko Oct 14, 2024

georgylobko Oct 14, 2024

just-boris Oct 14, 2024

		@@ -167,11 +167,18 @@ describe.each(['classic', 'refresh', 'refresh-toolbar'] as Theme[])('%s', theme
		test(
		'does not render notifications slot when it is empty',

fix: Empty notifications height #2856

fix: Empty notifications height #2856

Conversation

just-boris commented Oct 11, 2024 • edited Loading

Description

How has this been tested?

Correctness

Security

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 11, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

just-boris commented Oct 11, 2024 •

edited

Loading

codecov bot commented Oct 11, 2024 •

edited

Loading