[SideNav] Unit test coverage, refactor active state management

[weronikaolejniczak]

Summary

Resolves https://github.com/elastic/eui-private/issues/377

On this PR, I am adding unit tests that cover the whole side navigation functionality. See previous PRs:

• [SideNav] Move components from demo env to Storybook #228162
• [SideNav] Make logo clickable and minor improvements #230392
• [SideNav] Add badges and external link icon #230787

Changes

• Added test suites: both_modes.test.tsx (expanded + collapsed mode), collapsed_mode.test.tsx, expanded_mode.test.tsx.
• Refactored the active state management to be fully controlled by activeItemId, no internal state.
• Refactored isActive to be: isCurrent (aria-current="page") and isHighlighted (visual indication).
• Added onItemClick to the navigation component interface.
• Changed the primary menu limit to 12.
• Improved the roving index.

Motivation

The reason I'm adding the tests afterwards is because we were iterating fast, the requirements changed frequently and the acceptance criteria were not documented anywhere.

With this testing suite, I was able to refactor the active management state to be fully controlled through activeItemId prop without introducing regression.

Coverage

File	% Stmts	% Branch	% Funcs	% Lines	Uncovered Line #s
All files	78.17	71.47	70.77	79.48
core/packages/chrome/navigation	0	0	0	0
types.ts	0	0	0	0
.../packages/chrome/navigation/src	100	100	100	100
constants.ts	100	100	100	100
...rome/navigation/src/stories	0	0	0	0
navigation.stories.tsx	0	0	0	0	23-200
...chrome/navigation/src/tests	100	100	100	100
resize_window.ts	100	100	100	100
...hrome/navigation/src/components	84.9	82.85	92.85	84.9
navigation.tsx	84.9	82.85	92.85	84.9	...44-146,319-321
...omponents/nested_secondary_menu	94.87	69.23	100	94.87
header.tsx	100	100	100	100
menu_item.tsx	90	50	100	90	60
menu_panel.tsx	100	100	100	100
primary_menu_item.tsx	100	75	100	100	31-32
use_nested_menu.ts	83.33	50	100	83.33	24
...vigation/src/components/popover	95	90.32	88.23	100
blur_popover.ts	100	100	100	100
use_keyboard_management.ts	96.15	80	100	100	37,48-54
use_persistent_popover.ts	100	100	100	100
use_popover_hover.ts	100	100	100	100
use_popover_open.ts	80	100	60	100
...n/src/components/secondary_menu	100	80	100	100
item.tsx	100	70.58	100	100	42-47,61,86-100
section.tsx	100	100	100	100
...igation/src/components/side_nav	100	94	100	100
footer.tsx	100	100	100	100
footer_item.tsx	100	92.85	100	100	55
logo.tsx	100	100	100	100
panel.tsx	100	100	100	100
primary_menu.tsx	100	100	100	100
primary_menu_item.tsx	100	92.3	100	100	82-92
...ges/chrome/navigation/src/hooks	96.49	76.74	96	98.14
use_hover_timeout.ts	100	100	100	100
use_layout_width.ts	100	83.33	100	100	27
use_menu_header_style.ts	100	50	100	100	25-32
use_navigation.ts	100	100	100	100
use_responsive_menu.ts	94.73	68.75	80	94.44	47,63
use_roving_index.ts	94.73	81.81	100	100	25,56
use_tooltip.ts	100	100	100	100
...ges/chrome/navigation/src/utils	87.35	75	93.75	92
calculate_item_visible_count.ts	100	83.33	100	100	28
focus_first_element.ts	85.71	50	100	100	19-23
focus_main_content.ts	75	50	100	75	16
get_actual_item_heights.ts	100	100	100	100
get_focusable_elements.ts	100	100	100	100
get_has_submenu.ts	100	100	100	100
get_initial_active_items.ts	83.33	83.33	80	83.33	66-71
partition_menu_items.ts	100	100	100	100
trap_focus.ts	75	62.5	100	91.66	29
...kages/shared/kbn-i18n-react/src	58.33	25	33.33	58.33
compatiblity_layer.tsx	33.33	0	0	33.33	29-32,52
inject.tsx	0	0	0	0
provider.tsx	83.33	50	100	83.33	23
...rm/packages/shared/kbn-i18n/src	7.69	0	0	8.33
loader.ts	7.69	0	0	8.33	36-164
...ckages/shared/kbn-i18n/src/core	51.16	41.66	54.54	51.16
error_handler.ts	33.33	0	0	33.33	24-26
formats.ts	100	100	100	100
i18n.ts	51.28	45.45	60	51.28	...83,193,207-221
.../shared/kbn-test/src/jest/setup	88.88	88.88	100	88.88
emotion.js	88.88	88.88	100	88.88	43

Test Suites: 3 passed, 3 total
Tests: 73 passed, 73 total
Snapshots: 3 passed, 3 total
Time: 14.984 s, estimated 16 s

Checklist

Run node scripts/jest src/core/packages/chrome/navigation/src/ --coverage.

General

• Unit or functional tests were updated or added to match the most common scenarios
• The PR description includes the appropriate Release Notes section, and the correct release_note:* label is applied per the guidelines
• Review the backport guidelines and apply applicable backport:* labels.

Specific

• All added tests pass in CI. See below 👇🏻
• All added tests should pass locally. Run yarn test:jest src/core/packages/chrome/navigation to check.
• There should be a high coverage of unit tests in the navigation package. Run yarn test:jest src/core/packages/chrome/navigation --coverage to check.
• All side navigation acceptance criteria are covered. Verify with the Google Doc notebook, "Acceptance criteria" tab.

[elasticmachine]

Pinging @elastic/eui-team (EUI)

[elasticmachine]

⏳ Build in-progress, with failures

• Buildkite Build
• Commit: 6909aa2

Failed CI Steps

• Check Types

History

• 💔 Build #339481 failed cd10be1

cc @weronikaolejniczak

[Dosant]

I noticed that the tests are quite noisy with the following warnings:

console.error
      Error: Not implemented: navigation (except hash changes)
       

console.error
      Warning: An update to SideNavPopover inside a test was not wrapped in act(...).
      
      When testing, code that causes React state updates should be wrapped into act(...):
      
      act(() => {
        /* fire events that update state */
      });
      /* assert on the output */
      
      This ensures that you're testing the behavior the user would see in the browser. Learn more at https://reactjs.org/link/wrap-tests-with-act

If there is a way to get around them, would be great

[weronikaolejniczak]

@Dosant you are right, that is noisy. It happens on every anchor click. Let me try silencing it with some Jest mocking.

[Dosant]

Great job 👍 . I clicked around and everything seems to be working as expected on Kibana side without any additional changes.

The way controlled prop works now:

1. Kibana sets activeItemId based on current URL
2. User navigates to a different app (via nav or other way)
3. Url is updated
4. Kibana updates activeItemId

So we don't even need onItemClick for active state management. We will use it later for analytics

[acstll]

🚢