-
Notifications
You must be signed in to change notification settings - Fork 155
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: hide navigation button when toolbar variant of AppLayout has hideNavigation #2872
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2872 +/- ##
========================================
Coverage 96.21% 96.21%
========================================
Files 761 761
Lines 21490 21502 +12
Branches 7350 6963 -387
========================================
+ Hits 20676 20688 +12
- Misses 761 806 +45
+ Partials 53 8 -45 ☔ View full report in Codecov by Sentry. |
3645236
to
3f2d6ef
Compare
3f2d6ef
to
b472120
Compare
da33fc5
to
1568590
Compare
1568590
to
24d2456
Compare
//navigation must be null if hidden so toolbar knows to hide the toggle button | ||
const resolvedNavigation = navigationHide ? null : navigation ?? <></>; | ||
//navigation must not be open if navigationHide is true | ||
const resolvedNavigationOpen = !!resolvedNavigation && navigationOpen; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if no navigation provided, the fragment passed will be truthy, therefor resolvedNavigation will only be falsy if navigationHide is true
|
||
export default function () { | ||
const { | ||
urlParams: { navigationOpen = true, navigationHide = false }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so we can easily set expectations in regression tests
@@ -10,7 +10,7 @@ import { AppLayoutProps } from '../interfaces'; | |||
import { Focusable } from '../utils/use-focus-control'; | |||
import { SplitPanelToggleProps, ToolbarProps } from './toolbar'; | |||
|
|||
interface SharedProps { | |||
export interface SharedMultiAppLayoutProps { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only name changes for the interface and function
{...defaultAppLayoutProps} | ||
data-testid="first" | ||
navigation="testing nav" | ||
navigationHide={true} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The second layout using navigationHide is a more common case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the test name and added some assertions below to cover this case more thoroughly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small comments only
@@ -0,0 +1,109 @@ | |||
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | |||
// SPDX-License-Identifier: Apache-2.0 | |||
/* eslint-disable simple-import-sort/imports */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only do this in files which use src/app-layout/__tests__/utils.tsx
because it contains side effects.
All other tests can benefit from the linter rule
expect(firstLayout.findNavigation()).toBeFalsy(); | ||
expect(firstLayout.findNavigationToggle()).toBeFalsy(); | ||
expect(secondLayout.findNavigation()).toBeFalsy(); | ||
expect(secondLayout.findNavigationToggle()).toBeFalsy(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do we test here?
Is it about navigationHide
is respected, even if navigation
is defined? Then we should call it in the test name
Description
Overview
Other
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
CONTRIBUTING.md
.CONTRIBUTING.md
.Security
checkSafeUrl
function.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.