Skip to content
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

Merged
merged 6 commits into from
Oct 5, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions packages/components/src/navigator/context.js

This file was deleted.

12 changes: 12 additions & 0 deletions packages/components/src/navigator/context.ts
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 );
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,7 @@ The initial active path.

- Required: No


## The navigator object
## The `navigator` object

You can retrieve a `navigator` instance by using the `useNavigator` hook.

Expand Down

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(
Copy link
Contributor

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?

Copy link
Contributor Author

@ciampo ciampo Sep 30, 2021

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 nested NavigatorProvider.

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.

Copy link
Contributor

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...

Copy link
Contributor Author

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.

Copy link
Contributor

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).

Copy link
Contributor Author

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,

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).

So I'd vote for keeping the Context System in the Navigator component for now.

props,
'NavigatorProvider'
);

const [ path, setPath ] = useState< NavigatorPath >( {
path: initialPath,
} );

return (
<View ref={ forwardedRef } { ...otherProps }>
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having a wrapping div around all the "screens" made sense to me, and I think is going to be necessary to fix the scrollbar bug (flagged initially here) by setting overflow: hidden on it.

On top of that, it offers a component to attach a forwardedRef to, together with all of the remaining props that may be passed to this component (given that we mark its props as WordPressComponentProps).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like overflow: hidden as a way to solve the scrolling issue, we can't use as otherwise we may risk hiding popovers shown inside that div potentially. For me it's more a way to hide the issue and not solve it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking of applying overflow: hidden only while the transition is in progress (I have already a working proof of concept on my machine) — which shouldn't cause issues with modals & popovers.

I also spent some time looking into alternative solutions, but so far I haven't found anything better.

Copy link
Contributor

@stokesman stokesman Oct 4, 2021

Choose a reason for hiding this comment

The 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 navigatorScreens and that can't work reliably without one.

I don't think Marco is proposing we add overflow: hidden in this PR so we don't need to debate that here. I think it's going to be the solution but maybe not applied in the component itself or if so with a prop to control it.

Copy link
Contributor Author

@ciampo ciampo Oct 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think Marco is proposing we add overflow: hidden in this PR

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 div.

By having NavigatorProvider rendering a wrapper div, we would be in control of a root DOM element and its layout more explicitly (we could control its overflow, but also its display settings, its CSS containment, ...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I have already a working proof of concept on my machine)

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).
* 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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It connects the component to the Context System, so that the useContextSystem can be used to read from the context. It also does more, like forwarding refs, adding a display name and a CSS selector (see here).

It is a core functionality for the new generation of components that we've been building.

NavigatorProvider,
'NavigatorProvider'
);

export default ConnectedNavigatorProvider;
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,8 @@ Refer to [the `NavigatorProvider` component](/packages/components/src/navigator/

The component accepts the following props:

### `path`
### `path`: `string`

- Type: `string`
- Required: Yes
The screen's path, matched against the current path stored in the navigator.

The path of the current screen.
- Required: Yes
92 changes: 0 additions & 92 deletions packages/components/src/navigator/navigator-screen/component.js

This file was deleted.

Loading