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

Use <Navigator> component on Inspector Controls "back" button UX #45884

Open
getdave opened this issue Nov 18, 2022 · 21 comments
Open

Use <Navigator> component on Inspector Controls "back" button UX #45884

getdave opened this issue Nov 18, 2022 · 21 comments
Assignees
Labels
[Block] Navigation Affects the Navigation Block [Feature] Navigation Menus Any issue relating to Navigation Menus [Type] Enhancement A suggestion for improvement.

Comments

@getdave
Copy link
Contributor

getdave commented Nov 18, 2022

if the experiment is 👍 then proceed with refactor to utilise component to align with Global Styles.

In #45852 (comment) we added a "back" button to the Inspector Controls within the Nav block offcanvas experiment.

This is currently implemented in a relatively simplistic fashion with a simple <Button> which selects the parent block if one is available.

However, it would be preferable to mirror the same pattern as the Global Styles UI and utilise the <Navigator> component to create a hierarchy of panels with animation between each. This is likely more complex so should not be attempted unless the Offcanvas Nav experiment is approved, but it will produce a better UX.

@talldan and @jorgefilipecosta both suggested this PR as inspiration.

@youknowriad
Copy link
Contributor

That's a great opportunity to support dynamic screens in the "Navigator" component to avoid having to loop over all the blocks. cc @ciampo

<NavigatorScreen path="block/{clientId}"></NavigatorScreen>

@ciampo
Copy link
Contributor

ciampo commented Nov 18, 2022

Opened #45913 to track the "pattern matching" feature in Navigator

What the estimated timeline for this feature? Just trying to understand how to prioritize this task

@getdave
Copy link
Contributor Author

getdave commented Nov 21, 2022

What the estimated timeline for this feature? Just trying to understand how to prioritize this task

Currently we're working on a Nav block experiment (which is what is driving this) which we're hoping to have finalised by the end of this month.

If that experiment is successful in user testing then we'd be looking to merge the feature in WP 6.2. So ideally we'd need to have the panels system in place sufficiently in advance to allow us to implement in time 🙏

@getdave
Copy link
Contributor Author

getdave commented Nov 21, 2022

to avoid having to loop over all the blocks. cc @ciampo

@youknowriad Can I confirm that looping over all the blocks' <InsepctorControls> just so we can push them into a <Navigator> stack would be a poor choice?

Obviously it will "work" but I assume in an ideal world we would have a system whereby the "parent" can be dynamically added to the stack as a new block becomes selected and the history (.etc) will all "just work".

Am I understanding you correctly?

@youknowriad
Copy link
Contributor

@getdave yes, you're understanding correctly.

Looping over blocks is how the global styles already work so it didn't prove problematic so far but It's not ideal in terms of performance and code quality.

@ciampo
Copy link
Contributor

ciampo commented Nov 21, 2022

Just to bring some clarity on how Navigator works at the moment, and to recap together a few conversations that happened rencently:

Currently Navigator ’s logic is quite simple — the component was meant to be used in a scenario in which:

  • all possible NavigatorScreens are rendered as children of NavigatorProvider
  • the stack can be manipulated by pushing (ie navigate to next location) or popping (navigate back), very similarly to a browser’s history
  • pushing/popping can be done via the high level NavigatorButton and NavigatorBackButton components, or the lower-level useNavigator hook

Since all NavigatorScreen components are added as children of NavigationProvider, they will re-render (in the react sense) when props or context change — but only the selected screen will actually render DOM elements. In fact, the NavigatorScreen component will render null when it's not matching (and technically, there should always be either 1 or 0 matching screens at a given time — although we'd need to rework the component a little to enforce that).

I guess we’re also using dynamic to refer to two separate features:

  • Dynamically injecting “location items” in the Navigator’s history stack (ie. when selecting a block, adding a list of location items corresponding to the block’s ancestors, all the way up to the root block)
  • Dynamically matching a location path (i.e. blocks/a315h4298) to a NavigatorScreen’s path (ie. blocks/:block-id)

Looping over the tree of parent blocks in order to create all necessary NavigatorScreen instances may remove the need for feature no. 2 (to be confirmed), but we’ll still need to understand the best way to manipulare the history stack dynamically.

Let's work on refining the requirements first, which will help us to understand exactly what changes need to be done to Navigator in order to support this use case :)

@youknowriad
Copy link
Contributor

Dynamically injecting “location items” in the Navigator’s history stack (ie. when selecting a block, adding a list of location items corresponding to the block’s ancestors, all the way up to the root block)

How is this different than a navigator.goTo ? I feel there's something I missing here :)

@ciampo
Copy link
Contributor

ciampo commented Nov 21, 2022

How is this different than a navigator.goTo ? I feel there's something I missing here :)

From what I understood, going from a block's controls to its parent block's controls is conceptually represented as a back navigation, which implies that the screen representing the parent block is the previous one in the history stack — and therefore we'd need to "prepopulate" Navigator's history with a list of location objects mapping all parent blocks, so that navigating back goes to the correct parent block.

Using push (aka goTo) would add a new location to the stack, therefore moving forward instead of backward in history.

Although I may definitely misunderstood some of the requirements — excuses in advance if that's the case :)

@youknowriad
Copy link
Contributor

From what I understood, going from a block's controls to its parent block's controls is conceptually represented as a back navigation, which implies that the screen representing the parent block is the previous one in the history stack. Using push (aka goTo) would add a new location to the stack, therefore moving forward instead of backward in history.

I guess that's true but can we solve this by just making the navigator a bit more smart. Meaning that going from a path that looks like /block/something into /block/something/blabla is going forward but going to /block is going back.

@youknowriad
Copy link
Contributor

Also, why is it important to differentiate between going back and going forward? I guess it's just for the animation right?

There's also a difference between:

  • go back: that means go to parent (which is what is is now I think in the navigator)
  • go back: that means go to the previous route (which is how the browser history works)

In other words, maybe "going back" for the navigator as implemented today is just a shortcut to "go to parent" and if it simplifies things to "remove it", we should consider it?

@ciampo
Copy link
Contributor

ciampo commented Nov 21, 2022

Also, why is it important to differentiate between going back and going forward? I guess it's just for the animation right?

There's also a difference between:

  • go back: that means go to parent (which is what is is now I think in the navigator)
  • go back: that means go to the previous route (which is how the browser history works)

Currently there is a difference between going back and forward, related to the fact that the location history data is internally represented as a stack — in that sense, this resembles the browser history:

  • goTo pushes a new location to the top of the history stack
  • goBack pops the current location away from the top of the stack

Visually, goTo and goBack are currently associated with animations suggesting the "back" / "forward" navigation.

In other words, maybe "going back" for the navigator as implemented today is just a shortcut to "go to parent" and if it simplifies things to "remove it", we should consider it?

I think we had this conversation before. If I understand correctly, you're suggesting a new implementation where any navigation is a "push", and where the meaning of "back" of "forward" is only given by the direction of the animation.

I don't think that's a good implementation for a few reasons:

  • if we want to still keep a location stack, that means that the stack will always grow, and never reduce in size. At that point, will it make sense at all to store a stack?
  • currently, when navigating back, Navigator is able restore focus on the element that was clicked. If every navigation is technically a "push" to the stack I don't think we'd be able to implement that in a simple way, since we wouldn't easily know the difference between navigating to a "new" screen, or navigating to the "previous" screen
  • it would require us to state explicitly the path of every Back button (instead of just navigating to whatever was the previous location), which means more complex logic and/or a lot of duplication when creating NavigatorScreen

@artemiomorales
Copy link
Contributor

I looked into implementing the <Navigator> here and it didn't seem to fit the use case very well.

In particular, needing to know the JSX structure upfront in order to implement the <Navigator>, in the context of React components that are dynamically updating, was a blocker, and was like trying to fit together pieces that don't match.

As far as I can tell, we also don't need to use the navigation history — all we need is an animation.

With that in mind, below is all the code that is required to implement the animation itself using Framer Motion, as implemented in #46342.

Screen Shot 2022-12-09 at 9 57 04 AM

	if (
		isOffCanvasNavigationEditorEnabled &&
		blockInspectorAnimationSettings
	) {
		const animationOrigin =
			blockInspectorAnimationSettings &&
			blockInspectorAnimationSettings.enterDirection === 'leftToRight'
				? -50
				: 50;

		return (
			<motion.div
				animate={ {
					x: 0,
					opacity: 1,
					transition: {
						ease: 'easeInOut',
						duration: 0.14,
					},
				} }
				initial={ {
					x: animationOrigin,
					opacity: 0,
				} }
				key={ selectedBlockClientId }
			>
				<BlockInspectorSingleBlock
					clientId={ selectedBlockClientId }
					blockName={ blockType.name }
				/>
			</motion.div>
		);
	}

Right now I've just configured an enter animation, but you could also configure an exit animation, and these can be triggered whenever a new block is selected by passing in the selectedBlockClientID as a key on <motion.div>. Using block settings, we can also configure what each individual inspector's animation should be at the block level.

With this implementation being so simple and being able to trigger these animations via prop updates, I'm not sure that we gain much by creating a hierarchy of panels and seemingly working against React for the purposes of getting an animation to work.

With that in mind, I can see a benefit to creating a wrapper around Framer Motion to allow developers to configure their own enter and exit animations for their block inspectors, and perhaps the <Navigator> can be revised to fit this purpose, with the panel hierarchy and navigation history aspects of it made optional.

There may also be accessibility concerns with this approach I haven't considered, which, from my understanding, we can explore based on how the user tests go.

Would love to hear any additional perspectives on this!

@getdave
Copy link
Contributor Author

getdave commented Dec 12, 2022

@artemiomorales I found exactly the same issue when I tried to implement using Navigator. Square peg, round hole.

What's the goal here? Introduce an animation. In that case - as you suggest - why do we need to "navigate" between panels in a hierarchy?

I think the concern is that we'll be introducing a new animation but perhaps we can use CSS custom props or SaSS vars to ensure they are standardised?

@youknowriad
Copy link
Contributor

In particular, needing to know the JSX structure upfront in order to implement the , in the context of React components that are dynamically updating, was a blocker,

What's the issue here, can you clarify a bit more?

For me it's not about recreating the animation, it's about a component that was created for a given purpose (which corresponds here) but it's not fulfilling its promise, so we might want to adapt it.

@getdave
Copy link
Contributor Author

getdave commented Dec 12, 2022

@youknowriad From my memory (and @artemiomorales can confirm) it was because Navigator needs all the components to be (pre)rendered in a hierarchy so you can navigate between them. So in this case you'd have to render all the inspector controls for all the current block's ancestors. Then you can move backwards through the history.

If you try and do things dynamically as it stands it's not possible. I think I tried rendering the current block and it's immediate parent only and then Navigating backwards. But the issue is once you navigate to the parent there is nowhere left to go.

What we need is a means to dynamically generate the "next" parent upon navigate.

@artemiomorales to confirm though as he's now closer to this than I am.

@youknowriad
Copy link
Contributor

So I agree that having "dynamic screens" like suggested on my comment above would be a great addition #45884 (comment)

But Just noting that doing this:

{ blocks.map( clientId =>  <NavigatorScreen path={`blocks/${clientId}`}><BlockInspector clientId={ clientId } /></NavigatorScreen> ) }

This won't render all the inspector controls of all blocks, the NavigatorScreen component will ensure that if a path is not active, it's not rendered. That's already how it's done in the Global Styles panel.

In other words, yeah dynamic routes will be a great addition and optimization but doing navigation is already possible even for a long list of static paths.

There are some subtle things about the "back" and the history though (mentioned in this comment #45884 (comment))

@artemiomorales
Copy link
Contributor

artemiomorales commented Dec 16, 2022

What's the issue here, can you clarify a bit more?

For me it's not about recreating the animation, it's about a component that was created for a given purpose (which corresponds here) but it's not fulfilling its promise, so we might want to adapt it.

@youknowriad I've created a draft pull request to illustrate my current understanding of how to implement the Navigator and continue discussion. Namely, I believe we'd need to introduce a special use case for the Navigation blocks, as the logic of the Navigator runs contrary to how rendering is handled in the Block Inspector.

I believe it may be possible to get the Navigator to work, but as noted in the pull request, we would also need to ensure this works well with block selection via the Document List view and canvas in addition to the off-canvas editor.

Would be appreciative of any ideas, feedback, and insights!

@scruffian
Copy link
Contributor

This might be worth revisiting now #47883 has merged.

@ciampo
Copy link
Contributor

ciampo commented Feb 13, 2023

You beat me to it, @scruffian !

As Ben said, we've recently introduced:

  • pattern matching (ie. /product/:id)
  • navigating to "parent screen" in Navigator (via the goToParent() function and the <NavigatorToParentButton /> component)

I believe that these new features should suit your needs too :)

@getdave
Copy link
Contributor Author

getdave commented Mar 17, 2023

@draganescu Did we decide on a next step here? I feel like you were looking into this.

@draganescu
Copy link
Contributor

The next step is to actually do it :D I have not started any work on this so it's up for grabs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Navigation Affects the Navigation Block [Feature] Navigation Menus Any issue relating to Navigation Menus [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

No branches or pull requests

7 participants