Navigator: refactor to TypeScript
#35214
Conversation
ciampo
added
[Feature] Component System
|
Size Change: +156 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
ciampo
requested review from
sarayourfriend,
mirka,
youknowriad and
diegohaz
| props: WordPressComponentProps< NavigatorProviderProps, 'div' >, | ||
| forwardedRef: Ref< any > | ||
| ) { | ||
| const { initialPath, children, ...otherProps } = useContextSystem( |
There was a problem hiding this comment.
Why do we need to use this context system?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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.
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.
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.
| } ); | ||
|
|
||
| return ( | ||
| <View ref={ forwardedRef } { ...otherProps }> |
There was a problem hiding this comment.
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.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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, ...)
There was a problem hiding this comment.
(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)
| * ); | ||
| * ``` | ||
| */ | ||
| const ConnectedNavigatorProvider = contextConnect( |
There was a problem hiding this comment.
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.
| >; | ||
|
|
||
| function NavigatorScreen( props: Props, forwardedRef: Ref< any > ) { | ||
| const { children, path, ...otherProps } = useContextSystem( |
There was a problem hiding this comment.
I guess the same question above applies here.
| // Props specific to `framer-motion` can't be currently passed to `NavigatorScreen`, | ||
| // as some of them would overlap with HTML props (e.g. `onAnimationStart`, ...) | ||
| type Props = Omit< | ||
| WordPressComponentProps< NavigatorScreenProps, 'div', false >, |
There was a problem hiding this comment.
What does WordPressComponentProps do? In other words, why NavigatorScreenProps is not enough as Props type for this component?
There was a problem hiding this comment.
This is the basic type that we've been using for all recently-built component's props (definition).
It augments NavigatorScreenProps with all of the props that a div HTML element may accept. In this case, the false means that we're marking NavigatorScreen as non-polymorphic.
There was a problem hiding this comment.
WordPressComponentProps wrapper cuts out a ton of tedious type code that we'd have to repeat for every component that spreads its props into the root element. It also covers polymorphism edge cases that would otherwise be difficult and even more tedious and repetative to apply to every polymorphic component.
|
|
||
| if ( prefersReducedMotion ) { | ||
| return ( | ||
| <View ref={ forwardedRef } { ...otherProps }> |
There was a problem hiding this comment.
What does View bring compared to div?
There was a problem hiding this comment.
View is the basic building block of the components library. Every component that has been build recently uses View under the hood. In its default state, it renders a div, but it then enables other features (like polymorphism)
There was a problem hiding this comment.
Looking at the code of the View component, it's not clear to me what that does bring to high-level components?
There was a problem hiding this comment.
There are a few practical advantages. The main is that View is an Emotion "styled" component, and thus it inherently enables styling (through Emotion and its APIs) and polymorphism when used under the hood.
In the past, View was also used to allow styles to be cached/applied across iframes (now we changed approach) — but this still shows how having View as a core building block can be very important.
In my opinion, it is also conceptually important that we use View and the base components of all our components — every component is a View at its core.
There was a problem hiding this comment.
The main advantage to View in my "view" of things (lol sorry) is that it allows polymorphism for components. Other advantages are trivial. Styling any element is already possible through the className API and we don't use Emotion's CSS prop, so that's not really a reason to use the View component. I think it comes down to that it allows for polymorphism.
Every comment was replied to. What should the next steps be on this PR? |
All of the points that you raised are more than valid, and have been discussed/tweaked numerous times before. It's always healthy to re-evaluate such technical decisions! I just think that in this PR we should go with what we've been doing in the rest of the package, and discuss/apply any potential changes separately. Thank you again for taking the time to review the code in depth! |
Description
Part of #34907
Changes in this PR:
Navigator*components to TypeScript (including adding thenavigator/**folder totsconfig.json)NavigatorProviderandNavigatorScreencomponents to the Context SystemThere are a few minor runtime changes, which were applied when connecting the components to the Context System (via the
useContextSystemhook):NavigatorProvidercomponent have been wrapped in aViewcomponentNavigationScreencomponent, whenprefersReducedMotion === true, aViewcomponent is now returned instead of adivHow has this been tested?
Types of changes
Refactor (from JavaScript to TypeScript)
Checklist:
*.native.jsfiles for terms that need renaming or removal).