Site editor: style the selected template pattern

[benazeer-ben]

What?

Fixes #65127

Enhance the template selector section by adding "selected" styling to active elements based on the issue raised in GitHub Ticket #65127.

Why?

This improvement addresses the issue raised in [GitHub Ticket #65127]

Users had difficulty identifying which templates they had selected. Adding styling to active elements provides a clearer and better user experience.

How?

To resolve this, two files were updated:

index.js [packages/block-editor/src/components/block-patterns-list/index.js]:
Modified the code to update the active pattern selection when a template is clicked. The is-active class is now added to active elements.

style.scss [packages/block-editor/src/components/block-patterns-list/style.scss ]:
Styled the selected templates by targeting the is-active class, visually distinguishing them from unselected ones.

The proposed solution in the ticket could not resolve the issue using data-active-item because it wasn’t updating when templates were clicked.
Hence, the JS code was updated to manage the active state and apply the is-active class on selected patterns. The styling in the CSS reflects these changes.

Testing Instructions

1. Navigate to the template section in the site editor.
2. Change the template for any page.
3. Select a template pattern from the design section.
4. The selected pattern will have a blue border to indicate its active state.

Testing Instructions for Keyboard

1. Navigate to the template section:
   Use the Tab key to move through the site editor interface until you reach the template section.

2. Change the template:
   Use the Tab key to navigate through the list of available templates.
   Once focused on a template, press Enter to select it.

3. Select a template pattern:
   Use Tab to navigate to the design section, where different template patterns are displayed.
   Use the arrow keys (Up, Down, Left, Right) to move between the available patterns.
   Press Enter to select a pattern. The active pattern should now be styled with a blue border.

Kindly let me know if anyone has any feedbacks or any improvements needed.

Thanks,
Benazeer

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Unlinked Accounts

The following contributors have not linked their GitHub and WordPress.org accounts: @benazeerhassan1909.

Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Unlinked contributors: benazeerhassan1909.

Co-authored-by: benazeer-ben <[email protected]>
Co-authored-by: ciampo <[email protected]>
Co-authored-by: ntsekouras <[email protected]>
Co-authored-by: t-hamano <[email protected]>
Co-authored-by: jasmussen <[email protected]>
Co-authored-by: patil-vipul <[email protected]>
Co-authored-by: juanfra <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[github-actions]

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @benazeer-ben! In case you missed it, we'd love to have you join us in our Slack community.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

[ciampo]

Hey @benazeer-ben , thank you so much for working on your first contribution to Gutenberg 🙏

I have some reservations about the proposed approach — I have left more details in each inline comment. I will also leave a comment in the original issue

[ciampo]

@benazeer-ben as there are still quite a few aspects that need addressing — at a high level:

• no need for extra onClick handlers, Composite.Item already has one;
• no need to add a tabindex=0, Composite already handles that
• the setSelectedPattern call already happens in the list component, no need to call the function in each item;
• selectedId and setSelectedId are not valid props of the Composite component;
• the is-selected class can be added directly to Composite.Item

I've pushed a draft PR where I applied some changes — you should take a look at the changes across those commits and apply them to this PR (I tried pushing the changes directly here, but didn't have permissions).

Alternatively, here's a diff of all the changes that I recommend you apply to this PR:

diff --git a/packages/block-editor/src/components/block-patterns-list/index.js b/packages/block-editor/src/components/block-patterns-list/index.js
index d19bacf31f..f95d6a3a50 100644
--- a/packages/block-editor/src/components/block-patterns-list/index.js
+++ b/packages/block-editor/src/components/block-patterns-list/index.js
@@ -42,8 +42,7 @@ function BlockPattern( {
 	showTitle = true,
 	showTooltip,
 	category,
-	isSelected, // new prop to manage active state
-	setActivePattern, // function to set the active pattern
+	isSelected,
 } ) {
 	const [ isDragging, setIsDragging ] = useState( false );
 	const { blocks, viewportWidth } = pattern;
@@ -79,10 +78,7 @@ function BlockPattern( {
 		>
 			{ ( { draggable, onDragStart, onDragEnd } ) => (
 				<div
-					className={ clsx(
-						'block-editor-block-patterns-list__list-item',
-						{ 'is-selected': isSelected } // Apply 'is-selected' class if this pattern is active
-					) }
+					className="block-editor-block-patterns-list__list-item"
 					draggable={ draggable }
 					onDragStart={ ( event ) => {
 						setIsDragging( true );
@@ -97,10 +93,6 @@ function BlockPattern( {
 							onDragEnd( event );
 						}
 					} }
-					onClick={ () => {
-						onClick( pattern, blocks );
-						onHover?.( null );
-					} }
 				>
 					<WithToolTip
 						showTooltip={
@@ -126,13 +118,13 @@ function BlockPattern( {
 												pattern.type ===
 													INSERTER_PATTERN_TYPES.user &&
 												! pattern.syncStatus,
+											'is-selected': isSelected,
 										}
 									) }
 								/>
 							}
 							id={ id }
 							onClick={ () => {
-								setActivePattern( id ); // Set active pattern when clicked
 								onClick( pattern, blocks );
 								onHover?.( null );
 							} }
@@ -223,17 +215,15 @@ function BlockPatternsList(
 
 		setActiveCompositeId( firstCompositeItemId );
 	}, [ shownPatterns, blockPatterns ] );
-	const handleClickPattern = ( pattern ) => {
-		setActivePattern( pattern.name ); // Set the clicked pattern as active
-		onClickPattern( pattern ); // Original onClick logic
+	const handleClickPattern = ( pattern, blocks ) => {
+		setActivePattern( pattern.name );
+		onClickPattern( pattern, blocks );
 	};
 	return (
 		<Composite
 			orientation={ orientation }
 			activeId={ activeCompositeId }
 			setActiveId={ setActiveCompositeId }
-			selectedId={ activePattern }
-			setSelectedId={ setActivePattern }
 			role="listbox"
 			className="block-editor-block-patterns-list"
 			aria-label={ label }
@@ -252,8 +242,9 @@ function BlockPatternsList(
 						showTitle={ showTitle }
 						showTooltip={ showTitlesAsTooltip }
 						category={ category }
-						isSelected={ activePattern === pattern.name } // Highlight the active pattern
-						setActivePattern={ setActivePattern } // Function to set the active pattern
+						isSelected={
+							!! activePattern && activePattern === pattern.name
+						}
 					/>
 				) : (
 					<BlockPatternPlaceholder key={ pattern.name } />
diff --git a/packages/block-editor/src/components/block-patterns-list/style.scss b/packages/block-editor/src/components/block-patterns-list/style.scss
index 57dd5893af..534f5a5fa2 100644
--- a/packages/block-editor/src/components/block-patterns-list/style.scss
+++ b/packages/block-editor/src/components/block-patterns-list/style.scss
@@ -14,13 +14,6 @@
 	&[draggable="true"] {
 		cursor: grab;
 	}
-
-	// Solution for "Add "selected" styling to selected pattern in the design selector."
-	// https: //github.com/WordPress/gutenberg/issues/65127
-
-	&.is-selected {
-		border: $border-width solid var(--wp-admin-theme-color);
-	}
 }
 
 .block-editor-block-patterns-list__item {
@@ -52,16 +45,21 @@
 		}
 	}
 
-	&:hover:not(:focus) .block-editor-block-preview__container::after {
-		outline-color: rgba($black, 0.3);
+	&:hover:not( [data-focus-visible] )
+		.block-editor-block-preview__container::after {
+		outline-color: rgba( $black, 0.3 );
 	}
 
-	&:focus .block-editor-block-preview__container::after {
-		outline-color: var(--wp-admin-theme-color);
-		outline-width: var(--wp-admin-border-width-focus);
-		outline-offset: calc((-1 * var(--wp-admin-border-width-focus)));
+	&[data-focus-visible] .block-editor-block-preview__container::after {
+		outline-color: var( --wp-admin-theme-color );
+		outline-width: var( --wp-admin-border-width-focus );
+		outline-offset: calc( ( -1 * var( --wp-admin-border-width-focus ) ) );
 		transition: outline 0.1s linear;
-		@include reduce-motion("transition");
+		@include reduce-motion( 'transition' );
+	}
+
+	&.is-selected .block-editor-block-preview__container::after {
+		background-color: rgba( var( --wp-admin-theme-color--rgb ), 0.04 );
 	}
 
 	.block-editor-patterns__pattern-details:not(:empty) {

I still recommend you go through the commits linked above one by one, to understand a bit better the reason why those changes where made.

Also, you should rebase on top of trunk (or merge trunk into this PR's branch) to make sure you include the latest changes from trunk.

I also reworked the styles a bit, so that the selected pattern is now highlighted with a semi-transparent overlay, similarly to what we do in other parts of the app — although I'm not 100% convinced (invoking @WordPress/gutenberg-design and @juanfra for feedback)

Kapture.2024-10-16.at.23.11.09.mp4

[benazeer-ben]

Hi @ciampo ,

Thanks for your detailed explanations in the PR. I've incorporated the changes you suggested. However, I wanted to clarify one point you mentioned:

"No need to add a tabindex=0, Composite already handles that."

From my testing, it seems that adding tabindex=0 ensures that the design pattern is highlighted while navigating through the items using the keyboard. Without it, only the first item in the pattern list receives focus.

I've attached a video for reference showing the behavior with and without tabindex=0. Could you please take a look and share your thoughts on this?

Screenshare.-.2024-10-18.4_54_55.PM.mp4

Thanks again for your insights!

Best regards,
Benazeer

[ciampo]

The list of pattern previews is a composite widget, and as such, users can navigate through its items via arrow keys. See the Composite component and the related MDN docs.

Does that explain it?

---

On a separate node, could you make sure you rebase on top of trunk (or merge trunk into this PR's branch) to make sure you include the latest changes from trunk? Thank you 🙏

[ciampo]

Also, following design feedback, you could apply these changes to tweak focus and hover styles

Click to expand

diff --git a/packages/block-editor/src/components/block-patterns-list/style.scss b/packages/block-editor/src/components/block-patterns-list/style.scss
index 534f5a5fa2..b7e75754b7 100644
--- a/packages/block-editor/src/components/block-patterns-list/style.scss
+++ b/packages/block-editor/src/components/block-patterns-list/style.scss
@@ -42,24 +42,29 @@
 			outline: $border-width solid rgba($black, 0.1);
 			outline-offset: -$border-width;
 			border-radius: $radius-medium;
+
+			transition: outline 0.1s linear;
+			@include reduce-motion( 'transition' );
 		}
 	}
 
-	&:hover:not( [data-focus-visible] )
-		.block-editor-block-preview__container::after {
+	// Selected
+	&.is-selected .block-editor-block-preview__container::after {
+		outline-color: $gray-900;
+		outline-width: var( --wp-admin-border-width-focus );
+		outline-offset: calc( -1 * var( --wp-admin-border-width-focus ) );
+	}
+
+	// Hover state
+	&:hover .block-editor-block-preview__container::after {
 		outline-color: rgba( $black, 0.3 );
 	}
 
+	// Focused state
 	&[data-focus-visible] .block-editor-block-preview__container::after {
 		outline-color: var( --wp-admin-theme-color );
 		outline-width: var( --wp-admin-border-width-focus );
-		outline-offset: calc( ( -1 * var( --wp-admin-border-width-focus ) ) );
-		transition: outline 0.1s linear;
-		@include reduce-motion( 'transition' );
-	}
-
-	&.is-selected .block-editor-block-preview__container::after {
-		background-color: rgba( var( --wp-admin-theme-color--rgb ), 0.04 );
+		outline-offset: calc( -1 * var( --wp-admin-border-width-focus ) );
 	}
 
 	.block-editor-patterns__pattern-details:not(:empty) {

[benazeer-ben]

Hi @ciampo ,

Updated the code with latest style changes.

Also, the branch enhancement/selected-template-style is up to date with trunk.

Thanks.

[ciampo]

Hey @benazeer-ben , sorry for the radio silence. I had a couple of things that kept me away from work.

I've rebased your PR, solved a few conflicts, and now I'm going to test changes. Hopefully we can land it soon!

[ciampo]

LGTM 🚀 The new styles follow the feedback shared in #65127 (comment)

@juanfra , are you ok to merge this PR with the current changes?

[t-hamano]

@benazeer-ben Thank you for your work!

Refresh and check:
After saving, refresh the page.
Use Tab and the arrow keys to navigate back to the design section and verify that the previously selected pattern still has the active styling.

This doesn't seem to work properly.

Looking at the code, the selected pattern is only managed in state. This means that if we reload the browser or reopen the Design tab, the component will be re-rendered and the pattern will no longer be selected.

Did this issue not occur early on in this PR?

[benazeer-ben]

Hi @t-hamano

Thanks for raising this!

During initial stage, we were handling this scenarios then based on the some discussions we decided to avoid it.

Please have a look on these discussions in this PR:
#65917 (comment)
#65917 (comment)

Sorry for the confusions that happened from the wrong testing instruction .I have modified the details for testing.

[t-hamano]

@benazeer-ben

Thanks for letting me know this! If the current behavior is what we intended, then it's fine 👍

[ciampo]

Hey @t-hamano , thank you for testing! Indeed, we discussed and concluded that we don't currently have a good way to persist and retrieve this information on page reload, and that we should explore a solution (if needed) separately. (context).

Based on your positive feedback about the rest of the PR, I'm going to go ahead and merge.

Thank you @benazeer-ben for the collaboration!

[juanfra]

Thank you! it's testing nicely to me