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

Toolbar: set toolbar variant only when in block toolbar #54868

Closed
ciampo opened this issue Sep 27, 2023 · 8 comments · Fixed by #55139
Closed

Toolbar: set toolbar variant only when in block toolbar #54868

ciampo opened this issue Sep 27, 2023 · 8 comments · Fixed by #55139
Assignees
Labels
General Interface Parts of the UI which don't fall neatly under other labels. [Package] Components /packages/components [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@ciampo
Copy link
Contributor

ciampo commented Sep 27, 2023

Yea, I couldn't identify why the default was applying is-alternate here, so setting undefined variant resulted in the standard shadow.

Since #51154, changes were made so that DropdownMenu and Dropdown components rendered inside Toolbar would automatically pick the toolbar variant — that is achieved through the internal components context system (see here) and it causes the popover to render with the is-alternate classname.

So, given the previous requirements, the popover is currently behaving like expected: it's rendering a black border because it's rendered inside a Toolbar component.

From what I understand, the requirements that we gathered initially were incomplete, and what we'd really want is the toolbar popover variant to be only applied in popovers opened from the block toolbar (and not from any toolbar).

Therefore, this issue might need to be resolved in a higher-level component. Otherwise, similar problems may occur when new popovers are added to the header in the future.

That is correct. I guess for now, we could keep the changes from this PR. At the same time, we could work on refining the design requirements once for all, and then apply them at the components level.

Originally posted by @ciampo in #54840 (comment)

@jordesign jordesign added [Type] Bug An existing feature does not function as intended General Interface Parts of the UI which don't fall neatly under other labels. labels Sep 27, 2023
@ciampo ciampo added the [Package] Components /packages/components label Oct 2, 2023
@brookewp
Copy link
Contributor

brookewp commented Oct 2, 2023

Hi @jasmussen! I saw you confirmed that the black border for the dropdown menu should only appear in the block toolbar. However, I wanted to check on a few other block-related menus to ensure we have the full requirements. My understanding is the black border is to add contrast over editor content, but some scenarios contradict my assumption:

Inline

Screenshot 2023-10-02 at 10 40 20 AM

Previews

Screenshot 2023-10-02 at 10 45 08 AM

List view settings

Screenshot 2023-10-02 at 10 44 11 AM

Small screens

The following appears to be correct, but I wanted to double-check since the toolbar border color changes on smaller screens:

Screenshot 2023-10-02 at 10 46 42 AM

@jasmussen
Copy link
Contributor

Hey @brookewp, thanks for checking in!

My understanding is the black border is to add contrast over editor content

No, the text inside each menu item is what's carrying the necessary contrast, the border around both block toolbars and popovers is decorative.

To that end this is more about elevation, or materials, so to speak. For that reason, the heuristic is that the dropdown menu should be made of the same material as the container that opened it. For example:

  • The floating block toolbar has a dark border, therefore all menus opened should have the same.
  • The docked top toolbar has a gray border, therefore all menus opened should be the shadowed/gray-bordered menus.

Makes sense? We can flex a bit, it's more of an optimization than a rush job to get this right.

@brookewp
Copy link
Contributor

brookewp commented Oct 3, 2023

the dropdown menu should be made of the same material as the container that opened it

Thanks for looking, @jasmussen! To confirm I understand - in the examples I shared above, all are correct except for the view on smaller screens since there is a black border around the menu while the Toolbar has the grey menu. Is that right?

@jasmussen
Copy link
Contributor

Yep, all those are correct except the view on the smaller screen. But I understand how that one is tricky because it's technically still the block toolbar, suddenly it just shifted position to be docked to the top. If that isn't easily fixed, that's okay, because we probably need to revisit mobile separately, and some different rules might come into play.

@ciampo
Copy link
Contributor Author

ciampo commented Oct 4, 2023

Given that the aim is to visually match the border of the dropdown, in a way the current (trunk) behaviour seems correct in that all dropdowns opened inside a Toolbar match its borders, which are dark by default.

The issue arises more from the fact that Toolbar styles are sometimes overridden around the editor — this is the case for both the header toolbar (see override), like discussed in #54840, and for the block toolbar on small viewports (see override).

A few ideas on how to move forward:

  1. I guess one way could be for the Toolbar component to also expose a variant prop, so that we can specify a "borderless" variant — when the borderless variant is on, we don't change the popover border.
  2. Alternatively, we could try to "override" popover styles in those two specific toolbar instances by using the context — although it feels like a hacker approach.
  3. An alternative to the previous approach would be to simply adopt the solution already implemented in Fix ToolSelector popover variant #54840 — ie. override the popover variant via the popoverProps prop

@brookewp
Copy link
Contributor

brookewp commented Oct 5, 2023

Thanks for sharing options, @ciampo!

when the borderless variant is on, we don't change the popover border

In this option, we'd still need context for the popover variant when the toolbar's borderless variant isn't present, right? Maybe something like:

diff --git a/packages/components/src/toolbar/toolbar/index.tsx b/packages/components/src/toolbar/toolbar/index.tsx
index b6e83dcf6d..b9c725e849 100644
--- a/packages/components/src/toolbar/toolbar/index.tsx
+++ b/packages/components/src/toolbar/toolbar/index.tsx
@@ -19,19 +19,26 @@ import type { ToolbarProps } from './types';
 import type { WordPressComponentProps } from '../../context';
 import { ContextSystemProvider } from '../../context';
 
-const CONTEXT_SYSTEM_VALUE = {
-	DropdownMenu: {
-		variant: 'toolbar',
-	},
-	Dropdown: {
-		variant: 'toolbar',
-	},
+const CONTEXT_SYSTEM_VALUE = ( variant: string | undefined ) => {
+	if ( variant !== undefined ) {
+		return {};
+	}
+
+	return {
+		DropdownMenu: {
+			variant: 'toolbar',
+		},
+		Dropdown: {
+			variant: 'toolbar',
+		},
+	};
 };
 
 function UnforwardedToolbar(
 	{
 		className,
 		label,
+		variant,
 		...props
 	}: WordPressComponentProps< ToolbarProps, 'div', false >,
 	ref: ForwardedRef< any >
@@ -55,10 +62,12 @@ function UnforwardedToolbar(
 	// `ToolbarGroup` already uses components-toolbar for compatibility reasons.
 	const finalClassName = classnames(
 		'components-accessible-toolbar',
-		className
+		className,
+		variant === undefined ? undefined : `is-${ variant }`
 	);
+
 	return (
-		<ContextSystemProvider value={ CONTEXT_SYSTEM_VALUE }>
+		<ContextSystemProvider value={ CONTEXT_SYSTEM_VALUE( variant ) }>
 			<ToolbarContainer
 				className={ finalClassName }
 				label={ label }

However, this doesn't help with the mobile scenario. In that case, I'm unsure how I'd approach it without using CSS and the specific classes/media queries already in place. 🤔 Thoughts?

@ciampo
Copy link
Contributor Author

ciampo commented Oct 5, 2023

Maybe something like:

Yup, something like that

However, this doesn't help with the mobile scenario. In that case, I'm unsure how I'd approach it without using CSS and the specific classes/media queries already in place. 🤔 Thoughts?

We'd just need to set the variant correctly — i took a quick look and it seems like there is already a isFixed prop that we could use — something like variant={ isFixed ? VARIANT_NAME_WITHOUT_BORDERS : undefined }. In case this wasn't an option and we had to resort to reacting to the viewport size, we could use something like the useMediaQuery hook hook. What do you think ?

@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Oct 6, 2023
@brookewp
Copy link
Contributor

brookewp commented Oct 6, 2023

We'd just need to set the variant correctly — i took a quick look and it seems like there is already a isFixed prop that we could use — something like variant={ isFixed ? VARIANT_NAME_WITHOUT_BORDERS : undefined }

Thank you! I was looking at the Toolbar in NavigableToolbar rather than at its usages, so this is very helpful. I've opened an issue with this solution: #55139

In case this wasn't an option and we had to resort to reacting to the viewport size, we could use something like the useMediaQuery hook hook. What do you think ?

I'm also glad you shared this info as I wasn't aware of this hook. Good to know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
General Interface Parts of the UI which don't fall neatly under other labels. [Package] Components /packages/components [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants