-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Navigator: simplify backwards navigation APIs #63317
Changes from 15 commits
3245197
ceffbf3
40bea11
147239c
b287e50
f232504
c4cdd9d
d716d27
1c40e2e
37af461
9988553
e8d1429
fbcd9be
8fbaebb
7650c06
8ed0484
a33aa8d
da79e35
f1f6888
c24eec9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,8 @@ | |
This feature is still experimental. “Experimental” means this is an early implementation subject to drastic and breaking changes. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds like we can remove the experimental callout now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was planning on removing those experimental callouts and Storybook tags once we also export the component without the experimental prefix (in a follow-up PR) |
||
</div> | ||
|
||
This component is deprecated. Please use the [`NavigatorBackButton`](/packages/components/src/navigator/navigator-back-button/README.md) component instead. | ||
|
||
The `NavigatorToParentButton` component can be used to navigate to a screen and should be used in combination with the [`NavigatorProvider`](/packages/components/src/navigator/navigator-provider/README.md), the [`NavigatorScreen`](/packages/components/src/navigator/navigator-screen/README.md) and the [`NavigatorButton`](/packages/components/src/navigator/navigator-button/README.md) components (or the `useNavigator` hook). | ||
|
||
## Usage | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,14 +19,13 @@ import { | |
NavigatorScreen, | ||
NavigatorButton, | ||
NavigatorBackButton, | ||
NavigatorToParentButton, | ||
useNavigator, | ||
} from '..'; | ||
import type { NavigateOptions } from '../types'; | ||
|
||
const INVALID_HTML_ATTRIBUTE = { | ||
raw: ' "\'><=invalid_path', | ||
escaped: " "'><=invalid_path", | ||
raw: '/ "\'><=invalid_path', | ||
escaped: "/ "'><=invalid_path", | ||
Comment on lines
+28
to
+29
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because of "going back" always behaving like "go to parent", all paths need to start with |
||
}; | ||
|
||
const PATHS = { | ||
|
@@ -148,23 +147,6 @@ function CustomNavigatorBackButton( { | |
); | ||
} | ||
|
||
function CustomNavigatorToParentButton( { | ||
onClick, | ||
...props | ||
}: Omit< ComponentPropsWithoutRef< typeof NavigatorBackButton >, 'onClick' > & { | ||
onClick?: CustomTestOnClickHandler; | ||
} ) { | ||
return ( | ||
<NavigatorToParentButton | ||
onClick={ () => { | ||
// Used to spy on the values passed to `navigator.goBack`. | ||
onClick?.( { type: 'goToParent' } ); | ||
} } | ||
{ ...props } | ||
/> | ||
); | ||
} | ||
ciampo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
const ProductScreen = ( { | ||
onBackButtonClick, | ||
}: { | ||
|
@@ -344,20 +326,20 @@ const MyHierarchicalNavigation = ( { | |
> | ||
{ BUTTON_TEXT.toNestedScreen } | ||
</CustomNavigatorButton> | ||
<CustomNavigatorToParentButton | ||
<CustomNavigatorBackButton | ||
onClick={ onNavigatorButtonClick } | ||
> | ||
{ BUTTON_TEXT.back } | ||
</CustomNavigatorToParentButton> | ||
</CustomNavigatorBackButton> | ||
</NavigatorScreen> | ||
|
||
<NavigatorScreen path={ PATHS.NESTED }> | ||
<p>{ SCREEN_TEXT.nested }</p> | ||
<CustomNavigatorToParentButton | ||
<CustomNavigatorBackButton | ||
onClick={ onNavigatorButtonClick } | ||
> | ||
{ BUTTON_TEXT.back } | ||
</CustomNavigatorToParentButton> | ||
</CustomNavigatorBackButton> | ||
<CustomNavigatorGoToBackButton | ||
path={ PATHS.CHILD } | ||
onClick={ onNavigatorButtonClick } | ||
|
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.
Should we just suggest
goBack()
? For someone attempting to usegoToParent
, it's not immediately clear in their IDE thatgoBack
is the recommended alternative.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
NavigatorBackButton
doesn't have agoBack
prop, since "going back" was already the default behavior. ThegoToParent
prop was changing that behaviour, but now that the default behaviour of "going back" is the same as "going to parent", that prop basically became a no-op and can just be omitted.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.
Ah, got it, makes sense.
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.
Actually thanks to your comment, I realised that this deprecation was not necessary because that was a code path that could not be hit anymore. I just removed the code instead: da79e35
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.
Perfect 🧹