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

Typography panel: Component upsizing #36230

Closed
2 of 7 tasks
mirka opened this issue Nov 4, 2021 · 6 comments
Closed
2 of 7 tasks

Typography panel: Component upsizing #36230

mirka opened this issue Nov 4, 2021 · 6 comments
Assignees
Labels
[Feature] Component System WordPress component system [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi [Package] Block editor /packages/block-editor [Package] Components /packages/components [Type] Tracking Issue Tactical breakdown of efforts across the codebase and/or tied to Overview issues.

Comments

@mirka
Copy link
Member

mirka commented Nov 4, 2021

On hold until #39397 is addressed.


Tasks required to cleanly migrate the Typography panel components to the new 40px sizes.

Prep work (merge at will)

Final steps to enable changes (merge all at once)

Component Enabling PR
FontAppearanceControl 🧪 #36162
FontFamilyControl 🧪 #TBD1
FontSizePicker #36162
LetterSpacingControl 🧪 #TBD1
LineHeightControl #36196
TextDecorationControl 🧪 #TBD2
TextTransformControl 🧪 #TBD2

(We can merge these into a temporary branch for testing before the final merge)

@mirka mirka added [Package] Components /packages/components [Type] Tracking Issue Tactical breakdown of efforts across the codebase and/or tied to Overview issues. [Package] Block editor /packages/block-editor [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi [Feature] Component System WordPress component system labels Nov 4, 2021
@mirka mirka self-assigned this Nov 4, 2021
@aaronrobertshaw
Copy link
Contributor

#TBD2: Replace TextTransformControl 🧪 and TextDecorationControl 🧪 implementation with ToggleGroupControl 🧪

I took a brief look at switching these typography controls to use ToggleGroupControl. The switch itself is easy enough assuming that we are ok replacing the current icons with simple text labels. There is a bit of a problem though in that the ToggleGroupControl is based on a RadioGroup so once you select a value you can't "toggle it off".

In the context of block supports, the ability to toggle it off was required to allow for any styles applied by the theme or global styles to take effect.

If it interests, here is a quick patch to see this in action
diff --git a/packages/block-editor/src/components/text-transform-control/index.js b/packages/block-editor/src/components/text-transform-control/index.js
index 262484d843..ede7f11571 100644
--- a/packages/block-editor/src/components/text-transform-control/index.js
+++ b/packages/block-editor/src/components/text-transform-control/index.js
@@ -1,29 +1,27 @@
 /**
  * WordPress dependencies
  */
-import { Button } from '@wordpress/components';
-import { __ } from '@wordpress/i18n';
 import {
-	formatCapitalize,
-	formatLowercase,
-	formatUppercase,
-} from '@wordpress/icons';
+	__experimentalToggleGroupControl as ToggleGroupControl,
+	__experimentalToggleGroupControlOption as ToggleGroupControlOption,
+} from '@wordpress/components';
+import { __ } from '@wordpress/i18n';
 
 const TEXT_TRANSFORMS = [
 	{
 		name: __( 'Uppercase' ),
+		label: 'AB',
 		value: 'uppercase',
-		icon: formatUppercase,
 	},
 	{
 		name: __( 'Lowercase' ),
+		label: 'ab',
 		value: 'lowercase',
-		icon: formatLowercase,
 	},
 	{
 		name: __( 'Capitalize' ),
+		label: 'Ab',
 		value: 'capitalize',
-		icon: formatCapitalize,
 	},
 ];
 
@@ -38,28 +36,22 @@ const TEXT_TRANSFORMS = [
  */
 export default function TextTransformControl( { value, onChange } ) {
 	return (
-		<fieldset className="block-editor-text-transform-control">
-			<legend>{ __( 'Letter case' ) }</legend>
-			<div className="block-editor-text-transform-control__buttons">
-				{ TEXT_TRANSFORMS.map( ( textTransform ) => {
-					return (
-						<Button
-							key={ textTransform.value }
-							icon={ textTransform.icon }
-							isSmall
-							isPressed={ value === textTransform.value }
-							aria-label={ textTransform.name }
-							onClick={ () =>
-								onChange(
-									value === textTransform.value
-										? undefined
-										: textTransform.value
-								)
-							}
-						/>
-					);
-				} ) }
-			</div>
-		</fieldset>
+		<ToggleGroupControl
+			value={ value }
+			onChange={ onChange }
+			label={ __( 'Letter case' ) }
+			isBlock
+		>
+			{ TEXT_TRANSFORMS.map( ( textTransform ) => {
+				return (
+					<ToggleGroupControlOption
+						key={ textTransform.value }
+						label={ textTransform.label }
+						aria-label={ textTransform.name }
+						value={ textTransform.value }
+					/>
+				);
+			} ) }
+		</ToggleGroupControl>
 	);
 }

@mirka
Copy link
Member Author

mirka commented Nov 22, 2021

once you select a value you can't "toggle it off"

Mmm, good point. I'm going to break this one out into a discrete issue (#36735). If there aren't any clear solutions, it might be better to keep the existing implementation for now.

@ciampo
Copy link
Contributor

ciampo commented Nov 22, 2021

Thank you for looking into this, @mirka !

The changes that you're proposing definitely go in a good direction — standardizing our UI!

I have a few considerations:

  • consistency across components: we should be careful about the "meaning" of a size prop, especially when used across many components:
    • what can a user expect when changing the size prop on a component? Different components may behave differently when the size prop is changed
    • should the prop's type be standardized across all components in the package? Should we use tshirt-like sizing options (e.g. 'small' | 'medium' | 'large' ...) or another scale?
  • I'd like to go even more in this direction and look at refactoring control components so that, ideally, they are all based on the same BaseControl (this came up originally in ToolsPanel: Standardize input control label margin #36387 (comment)
  • Regarding using experimental components inside stable components, it is something that should be discussed more in details. In general, if the experimental component remains an implementation detail (or is easy enough to replace later on), I don't see it as a problem.

@mirka
Copy link
Member Author

mirka commented Nov 30, 2021

  • consistency across components: we should be careful about the "meaning" of a size prop

I think a manageable scope to start with is to care about consistency within "form" elements only (e.g. buttons, inputs, selects, etc).

As for the actual size scale naming, my assessment of the current landscape and designer sentiment is that we're not at all ready to commit to anything 😅 (I attempted to establish a size scale beforehand but it turned out to be unfeasible.) I'm so hesitant to commit to size names at this stage, even the large identifier is prefixed with __unstable. There's a substantial chance that this size will end up being called something else. I think we'll have to manage with temporary size names until we figure out a unified size scale that works across contexts (block placeholders vs. sidebar).

Regarding using experimental components inside stable components, it is something that should be discussed more in details. In general, if the experimental component remains an implementation detail (or is easy enough to replace later on), I don't see it as a problem.

You're right, I guess that will be true in most cases, and like you said before it's pretty much unavoidable to use experimental components inside mature ones.

@ciampo
Copy link
Contributor

ciampo commented Nov 30, 2021

I think a manageable scope to start with is to care about consistency within "form" elements only (e.g. buttons, inputs, selects, etc).

Makes sense

As for the actual size scale naming, my assessment of the current landscape and designer sentiment is that we're not at all ready to commit to anything 😅 (I attempted to establish a size scale beforehand but it turned out to be unfeasible.) I'm so hesitant to commit to size names at this stage, even the large identifier is prefixed with __unstable. There's a substantial chance that this size will end up being called something else. I think we'll have to manage with temporary size names until we figure out a unified size scale that works across contexts (block placeholders vs. sidebar).

That's my fear too. Let's see how things evolve here, and let's consider other potential conventions too in case we can think about any alternative.

You're right, I guess that will be true in most cases, and like you said before it's pretty much unavoidable to use experimental components inside mature ones.

Agreed! Of course that doesn't mean that we shouldn't be mindful when using experimental components

@mirka
Copy link
Member Author

mirka commented Aug 15, 2022

Closing in favor of #41973

The Typography panel in Global Styles have been upsized in #42718. The ones in the post editor can also be upsized after TextTransformControl and TextDecorationControl are migrated to the component being prepared in #43060.

@mirka mirka closed this as completed Aug 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Component System WordPress component system [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi [Package] Block editor /packages/block-editor [Package] Components /packages/components [Type] Tracking Issue Tactical breakdown of efforts across the codebase and/or tied to Overview issues.
Projects
None yet
Development

No branches or pull requests

3 participants