feat: Public layout component #3208

georgylobko · 2025-01-21T10:22:03Z

Description

doc: tb1MA6m8ZZEt

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

How has this been tested?

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 · 2025-01-21T10:34:28Z

Codecov Report

Attention: Patch coverage is 98.43750% with 1 line in your changes missing coverage. Please review.

Project coverage is 96.43%. Comparing base (d6733f7) to head (587732c).

Files with missing lines	Patch %	Lines
src/app-layout-toolbar/index.tsx	97.56%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3208   +/-   ##
=======================================
  Coverage   96.42%   96.43%           
=======================================
  Files         791      793    +2     
  Lines       22605    22659   +54     
  Branches     7814     7839   +25     
=======================================
+ Hits        21797    21851   +54     
  Misses        754      754           
  Partials       54       54

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

georgylobko · 2025-01-21T11:06:37Z

pages/page-layout/styles.scss

These styles were copied from pages/app-layout

…mponent # Conflicts: # src/__tests__/snapshot-tests/__snapshots__/test-utils-wrappers.test.tsx.snap

…vided nor at least one drawer with trigger

…mponent

…mponent # Conflicts: # src/app-layout/visual-refresh-toolbar/multi-layout.ts

just-boris · 2025-02-13T12:38:00Z

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

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


This content is not connected to anything in app layout, so I think it is fine to reuse it instead of copying

just-boris · 2025-02-13T12:43:34Z

src/split-panel/implementation.tsx

-  const isToolbar = useAppLayoutToolbarEnabled();
+  const isToolbarPrivate = useAppLayoutToolbarEnabled();
+  const isToolbarPublic = useContext(AppLayoutToolbarPublicContext) ?? false;
+  const isToolbar = isToolbarPublic || isToolbarPrivate;


Did you count how many times we the combined condition isToolbarPublic || isToolbarPrivate? Would it be better to have a reusable utility for this too?

I think it's a case when keeping this condition explicit would be more helpful than abstracting it out. Do you think it's worth to do it?

Yes, it is definitely worth to have a reusable utility that gives you "we are under app layout toolbar" value with a one-line import

just-boris · 2025-02-13T12:45:36Z

src/__tests__/functional-tests/ssr.test.ts

+beforeEach(() => {
+  globalWithFlags[Symbol.for('awsui-visual-refresh-flag')] = () => true;
+});
+
+afterEach(() => {
+  delete globalWithFlags[Symbol.for('awsui-visual-refresh-flag')];
+  clearVisualRefreshState();
+});


I think these changes in src/__tests__/** files is another instance of something to extract as a separate PR and merge before the main one lands.

P.S. I think we also need to send a PSA to our team that the default behavior of these tests changed

These conditions (set refresh feature flag before running the tests suites) make sense only for this PR as it checks the AppLayoutToolbar component for classic theme. What exactly would you suggest to do? Implement these feature flag via useVisualRefresh and then change it as a follow-up PR to separate concerns?

On the one hand it makes sense, on the other hand, I would go ahead as is and let our team know about this decision. WDYT?

With this change we switch tests to use VR version by default, for all components. not only app layout.

So I suggest making this switch as a separate PR

Switched to useVisualRefresh. I'll switch it back to globalWithFlags[Symbol.for('awsui-visual-refresh-flag')] in a separate PR

Discussed offline, let's undo the change about useVisualRefresh and keep it this way until the PR is merged

…ayout

…] to useVisualRefresh in functional tests

just-boris · 2025-02-18T10:58:04Z

src/app-layout/visual-refresh-toolbar/multi-layout.ts

@@ -7,6 +7,7 @@ import { warnOnce } from '@cloudscape-design/component-toolkit/internal';
 import { awsuiPluginsInternal } from '../../internal/plugins/api';
 import { RegistrationState } from '../../internal/plugins/controllers/app-layout-widget';
 import { AppLayoutProps } from '../interfaces';
+import { useAppLayoutToolbarEnabled } from '../utils/feature-flags';


Let's rename these hooks

useAppLayoutFlagEnabled – the feature flags are active (for all advanced deduplication features)

useAppLayoutToolbarDesignEnabled - same as above, but also true when toolbar is used standalone

just-boris · 2025-02-18T11:07:27Z

src/__tests__/functional-tests/ssr.test.ts

+beforeEach(() => {
+  globalWithFlags[Symbol.for('awsui-visual-refresh-flag')] = () => true;
+});
+
+afterEach(() => {
+  delete globalWithFlags[Symbol.for('awsui-visual-refresh-flag')];
+  clearVisualRefreshState();
+});


Discussed offline, let's undo the change about useVisualRefresh and keep it this way until the PR is merged

…h-flag')] to useVisualRefresh in functional tests" This reverts commit 273633b.

georgylobko added 2 commits January 21, 2025 11:21

feat: PageLayout component

b08c0c2

chore: U tests for trigerless navigation and drawers

7db9b3d

chore: Update snapshots

610983d

georgylobko deployed to dev-pages January 21, 2025 10:49 — with GitHub Actions View deployment

georgylobko commented Jan 21, 2025

View reviewed changes

pages/page-layout/styles.scss Outdated

Copy link

Contributor Author

georgylobko Jan 21, 2025

Choose a reason for hiding this comment

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

These styles were copied from pages/app-layout

georgylobko added 2 commits January 21, 2025 12:15

chore: Clean up a testing page for page-layout

060df7f

chore: App layout internal props refactoring

3646600

georgylobko deployed to dev-pages January 21, 2025 11:22 — with GitHub Actions View deployment

georgylobko deployed to dev-pages January 21, 2025 11:29 — with GitHub Actions View deployment

georgylobko added 2 commits January 21, 2025 12:57

Merge remote-tracking branch 'origin/main' into feat/public-layout-co…

457fe76

…mponent # Conflicts: # src/__tests__/snapshot-tests/__snapshots__/test-utils-wrappers.test.tsx.snap

chore: Document optional trigger scenario for PageLayout

1074be4

georgylobko deployed to dev-pages January 21, 2025 12:24 — with GitHub Actions View deployment

chore: Document optional trigger scenario for PageLayout

ce4393f

georgylobko deployed to dev-pages January 21, 2025 13:13 — with GitHub Actions View deployment

chore: Don't display the toolbar if neither navigation trigger is pro…

b6fd838

…vided nor at least one drawer with trigger

georgylobko deployed to dev-pages January 21, 2025 14:30 — with GitHub Actions View deployment

Merge branch 'main' into feat/public-layout-component

e164d82

georgylobko deployed to dev-pages January 21, 2025 14:58 — with GitHub Actions View deployment

chore: Omit disableBodyScroll prop

a77bf17

georgylobko deployed to dev-pages January 23, 2025 16:20 — with GitHub Actions View deployment

georgylobko and others added 2 commits January 24, 2025 10:27

feat: Set useAppLayoutToolbarEnabled to true for the public layout co…

f61d5c0

…mponent

Merge branch 'main' into feat/public-layout-component

63e9618

georgylobko deployed to dev-pages January 24, 2025 09:40 — with GitHub Actions View deployment

georgylobko and others added 6 commits January 24, 2025 11:24

feat: focusNavigation API

0cf8021

chore: focusNavigation on without-toolbar.page.tsx

05bdf0d

chore: toolbarHide API

604a5bc

chore: Cleanup

b5e432b

Merge branch 'main' into feat/public-layout-component

3a5ba52

chore: Update snapshots

dffa826

georgylobko deployed to dev-pages February 12, 2025 09:53 — with GitHub Actions View deployment

georgylobko added 2 commits February 12, 2025 17:12

Merge remote-tracking branch 'origin/main' into feat/public-layout-co…

8760dd5

…mponent # Conflicts: # src/app-layout/visual-refresh-toolbar/multi-layout.ts

fix: Remove redundant navigationTriggerHide

61b009c

georgylobko deployed to dev-pages February 12, 2025 16:24 — with GitHub Actions View deployment

Merge branch 'main' into feat/public-layout-component

867f79f

georgylobko deployed to dev-pages February 12, 2025 16:37 — with GitHub Actions View deployment

chore: Small refactoring

5a2c9eb

georgylobko deployed to dev-pages February 12, 2025 16:45 — with GitHub Actions View deployment

chore: Small refactoring

785c664

georgylobko deployed to dev-pages February 12, 2025 17:07 — with GitHub Actions View deployment

georgylobko requested a review from just-boris February 13, 2025 09:42

just-boris reviewed Feb 13, 2025

View reviewed changes

georgylobko and others added 3 commits February 13, 2025 14:52

chore: Reuse pages/utils in pages/app-layout-toolbar from pages/app-l…

cf3ef8e

…ayout

chore: Remove styles.scss from pages/app-layout-toolbar

c9ace4e

Merge branch 'main' into feat/public-layout-component

6178811

georgylobko deployed to dev-pages February 13, 2025 14:06 — with GitHub Actions View deployment

georgylobko requested a review from just-boris February 17, 2025 12:40

Merge branch 'main' into feat/public-layout-component

8d306d9

georgylobko deployed to dev-pages February 17, 2025 12:48 — with GitHub Actions View deployment

chore: Switch globalWithFlags[Symbol.for('awsui-visual-refresh-flag')…

273633b

…] to useVisualRefresh in functional tests

georgylobko deployed to dev-pages February 18, 2025 09:58 — with GitHub Actions View deployment

georgylobko and others added 2 commits February 18, 2025 11:05

chore: Refactoring. Introduce useAppLayoutToolbarPublicEnabled hook

c21e321

Merge branch 'main' into feat/public-layout-component

5b0b82f

georgylobko deployed to dev-pages February 18, 2025 10:14 — with GitHub Actions View deployment

just-boris reviewed Feb 18, 2025

View reviewed changes

georgylobko and others added 3 commits February 18, 2025 14:54

Revert "chore: Switch globalWithFlags[Symbol.for('awsui-visual-refres…

2b0606c

…h-flag')] to useVisualRefresh in functional tests" This reverts commit 273633b.

chore: Refactoring. Rename feature flags

5e19c40

Merge branch 'main' into feat/public-layout-component

587732c

georgylobko deployed to dev-pages February 19, 2025 09:35 — with GitHub Actions View deployment

georgylobko requested a review from just-boris February 19, 2025 10:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Public layout component #3208

feat: Public layout component #3208

georgylobko commented Jan 21, 2025 •

edited

Loading

codecov bot commented Jan 21, 2025 •

edited

Loading

georgylobko Jan 21, 2025

just-boris Feb 13, 2025

georgylobko Feb 13, 2025

just-boris Feb 13, 2025

georgylobko Feb 13, 2025

just-boris Feb 17, 2025

georgylobko Feb 18, 2025

just-boris Feb 13, 2025 •

edited

Loading

georgylobko Feb 17, 2025

just-boris Feb 17, 2025

georgylobko Feb 18, 2025

just-boris Feb 18, 2025

just-boris Feb 18, 2025

georgylobko Feb 19, 2025

just-boris Feb 18, 2025

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

feat: Public layout component #3208

Are you sure you want to change the base?

feat: Public layout component #3208

Conversation

georgylobko commented Jan 21, 2025 • edited Loading

Description

How has this been tested?

Correctness

Security

Testing

codecov bot commented Jan 21, 2025 • 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

just-boris Feb 13, 2025 • edited Loading

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

georgylobko commented Jan 21, 2025 •

edited

Loading

codecov bot commented Jan 21, 2025 •

edited

Loading

just-boris Feb 13, 2025 •

edited

Loading