-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
: refactor to TypeScript
#35214
Changes from all commits
7375bee
b0cb7fa
cb30273
c02defa
60a510c
fee3f2d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { createContext } from '@wordpress/element'; | ||
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
import type { NavigatorContext as NavigatorContextType } from './types'; | ||
|
||
const initialContextValue: NavigatorContextType = [ {}, () => {} ]; | ||
export const NavigatorContext = createContext( initialContextValue ); |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,96 @@ | ||
/** | ||
* External dependencies | ||
*/ | ||
// eslint-disable-next-line no-restricted-imports | ||
import type { Ref } from 'react'; | ||
|
||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { useState } from '@wordpress/element'; | ||
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
import { | ||
contextConnect, | ||
useContextSystem, | ||
WordPressComponentProps, | ||
} from '../../ui/context'; | ||
import { View } from '../../view'; | ||
import { NavigatorContext } from '../context'; | ||
import type { NavigatorProviderProps, NavigatorPath } from '../types'; | ||
|
||
function NavigatorProvider( | ||
props: WordPressComponentProps< NavigatorProviderProps, 'div' >, | ||
forwardedRef: Ref< any > | ||
) { | ||
const { initialPath, children, ...otherProps } = useContextSystem( | ||
props, | ||
'NavigatorProvider' | ||
); | ||
|
||
const [ path, setPath ] = useState< NavigatorPath >( { | ||
path: initialPath, | ||
} ); | ||
|
||
return ( | ||
<View ref={ forwardedRef } { ...otherProps }> | ||
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. Why do we show a view at all, this is probably going to create an extra div for nothing. 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. Having a wrapping On top of that, it offers a component to attach a 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 don't like 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 thinking of applying I also spent some time looking into alternative solutions, but so far I haven't found anything better. 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. My argument for a wrapper would stem from wanting to animate the exits of I don't think Marco is proposing we add 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.
That is true (we should be tacking that in a separate PR), but at the same time, it is also one of the reasons for introducing a wrapper By having 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've refined my working proof and opened #35317 (which is based on top of this PR) |
||
<NavigatorContext.Provider value={ [ path, setPath ] }> | ||
{ children } | ||
</NavigatorContext.Provider> | ||
</View> | ||
); | ||
} | ||
|
||
/** | ||
* The `NavigatorProvider` component allows rendering nested panels or menus (via the `NavigatorScreen` component) and navigate between these different states (via the `useNavigator` hook). | ||
ntsekouras marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* The Global Styles sidebar is an example of this. The `Navigator*` family of components is _not_ opinionated in terms of UI, and can be composed with any UI components to navigate between the nested screens. | ||
* | ||
* @example | ||
* ```jsx | ||
* import { | ||
* __experimentalNavigatorProvider as NavigatorProvider, | ||
* __experimentalNavigatorScreen as NavigatorScreen, | ||
* __experimentalUseNavigator as useNavigator, | ||
* } from '@wordpress/components'; | ||
* | ||
* function NavigatorButton( { | ||
* path, | ||
* isBack = false, | ||
* ...props | ||
* } ) { | ||
* const navigator = useNavigator(); | ||
* return ( | ||
* <Button | ||
* onClick={ () => navigator.push( path, { isBack } ) } | ||
* { ...props } | ||
* /> | ||
* ); | ||
* } | ||
* | ||
* const MyNavigation = () => ( | ||
* <NavigatorProvider initialPath="/"> | ||
* <NavigatorScreen path="/"> | ||
* <p>This is the home screen.</p> | ||
* <NavigatorButton isPrimary path="/child"> | ||
* Navigate to child screen. | ||
* </NavigatorButton> | ||
* </NavigatorScreen> | ||
* | ||
* <NavigatorScreen path="/child"> | ||
* <p>This is the child screen.</p> | ||
* <NavigatorButton isPrimary path="/" isBack> | ||
* Go back | ||
* </NavigatorButton> | ||
* </NavigatorScreen> | ||
* </NavigatorProvider> | ||
* ); | ||
* ``` | ||
*/ | ||
const ConnectedNavigatorProvider = contextConnect( | ||
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. What is this for? 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. It connects the component to the Context System, so that the It is a core functionality for the new generation of components that we've been building. |
||
NavigatorProvider, | ||
'NavigatorProvider' | ||
); | ||
|
||
export default ConnectedNavigatorProvider; | ||
ciampo marked this conversation as resolved.
Show resolved
Hide resolved
|
This file was deleted.
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.
Why do we need to use this context system?
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.
Using the context system can be useful for the future, so that this component can read from it — for example, I can see how a parent
NavigatorProvider
could "sync" with a nestedNavigatorProvider
.The contest system is a core functionality for the new generation of components that we've been building. Using it here would also align this component to what we recommend in the contributing guidelines for new components.
But I also understand if at this point we may think this is not needed.
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 guess I'm usually in the camp that if you don't need it now, don't add it because you don't know what you'll want to do in the future, priorities can change...
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.
As I said previously, I get your point of view on the context system.
I'd personally like to hear a few more opinions (maybe @diegohaz and @sarayourfriend ) so that we can make a better-informed decision here.
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 think the idea behind using the context system everywhere is that it opens a uniform internal API for modifying component props contextually without introducing cross-component context dependencies. Doing it everywhere means the context system "just works"...
however, that being said, there are very few actual applications for this context system. It may be a case of overengineering a broad solution for a handful of use cases. However it does eliminate cross component context dependencies in those specific examples (ButtonGroup for example, does not create a dependency in Button on a ButtonGroupContext of any sort, meaning pulling in Button does not inherently mean the ButtonGroup dependency is also included).
Given the narrow scope of actual use cases for this context system, it might be worth re-evaluating if it should actually be applied everywhere. It's something that the component system should either fully commit to or leave by the wayside 🤷♀️ It definitely should not be only partially applied though (I think that would introduce too much complexity for the few use cases without creating a broad possibility of application in the future).
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 also agree on the need for re-assessing the utility Context System for the overall
@wordpress/components
package. But, as Sara said,So I'd vote for keeping the Context System in the
Navigator
component for now.