fix: Refactor and fix AppLayout and side navigation panel state and behaviour

[rubencarvalho]

Description

When clicking a navigation link in mobile AppLayout side navigation, the panel should close automatically. Furthermore, on mobile viewports, the side navigation should also be closed by default.

This used to be handled in our code, but has regressed:

This behavior is supposed to be handled by the AppLayout.
However, our side navigation was blocking this code. I've split the event listener to be applied to both the header text and trigger button for the expandable section, so we don't need the stopPropagation.

There was also a unit test for it, but very artificial and this is why the regression was not caught:

components/src/app-layout/__tests__/mobile.test.tsx

Line 85 in 715264b

test('closes navigation when clicking on links', () => {

Related links: AWSUI-21892

How has this been tested?

Existing tests, updated some "artificial ones" and added new ones to cover the expected behaviour.

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]

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (99e8bbf) 93.78% compared to head (4c020a8) 93.79%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1434   +/-   ##
=======================================
  Coverage   93.78%   93.79%           
=======================================
  Files         639      639           
  Lines       17228    17231    +3     
  Branches     5665     5667    +2     
=======================================
+ Hits        16158    16161    +3     
  Misses        997      997           
  Partials       73       73           

Files Changed	Coverage Δ
...c/expandable-section/expandable-section-header.tsx	100.00% <ø> (ø)
src/side-navigation/internal.tsx	100.00% <ø> (ø)
src/app-layout/index.tsx	92.83% <100.00%> (+0.05%)	⬆️
src/app-layout/visual-refresh/context.tsx	89.36% <100.00%> (+0.15%)	⬆️

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

[just-boris]

As opposed to adding more eslint-disable-next-line exceptions, I suggest converting const onNavigationToggle = useCallback(() => {}, []) to const onNavigationToggle = useStableCallback(() => {}). This value will be safe to put into effects

[timogasda]

Can we do this without an internal prop?

For example, what if we baked this behavior into the navigation variant of expandable section?

[rubencarvalho]

I've changed from using an internal prop (for detecting the desired behaviour of the trigger to expand the section) into the "navigation" variant of the expandable section.

navigation - Use this variant in the navigation panel with anchors and custom styled content. It doesn't have any default styles.

This is a more impactful change, but between a successful dry-run (ID: 6564992503) and our current guidelines (above), I'm confident this is the best approach.

[timogasda]

Let's do it!

[timogasda]

minor: Instead of casting the value of the variable (technically not safe), it would be better to cast the array that you iterate over (const size of ['desktop', 'mobile'] as const)

[rubencarvalho]

<ExpandableNavigationHeader /> is rendered conditionally only for the navigation variant, so the change below is only applied to this variant.

components/src/expandable-section/expandable-section-header.tsx

Lines 255 to 265 in 4c020a8

	if (variant === 'navigation') {
	return (
	<ExpandableNavigationHeader
	className={clsx(className, wrapperClassName)}
	ariaLabelledBy={ariaLabelledBy}
	{...defaultHeaderProps}
	>
	{headerText ?? header}
	</ExpandableNavigationHeader>
	);
	}