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.

Sign up for GitHub

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

Jump to bottom

Adding basic ordering to tab slot fill #38081

Merged
merged 5 commits into from May 4, 2023
Merged

Adding basic ordering to tab slot fill #38081

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
95c05e0
Adding basic ordering to tab slot fill
joelclimbsthings May 2, 2023
2c17cd2
Adding changelog
joelclimbsthings May 2, 2023
ee08a26
Adding plugin changelog
joelclimbsthings May 2, 2023
b36dddf
Fixing linting
joelclimbsthings May 3, 2023
a082ef4
Fixing issue with initial tab not being selected
joelclimbsthings May 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/js/product-editor/changelog/fix-tabs-reordering-38002
View file
Open in desktop
@@ -0,0 +1,4 @@
Significance: minor
Type: add

Adding basic ordering to product tabs slot-fill.
3 changes: 3 additions & 0 deletions packages/js/product-editor/src/blocks/tab/block.json
View file
Open in desktop
Expand Up @@ -13,6 +13,9 @@
},
"title": {
"type": "string"
},
"order": {
"type": "number"
}
},
"supports": {
Expand Down
4 changes: 2 additions & 2 deletions packages/js/product-editor/src/blocks/tab/edit.tsx
View file
Open in desktop
Expand Up @@ -21,7 +21,7 @@ export function Edit( {
};
} ) {
const blockProps = useBlockProps();
const { id, title } = attributes;
const { id, title, order } = attributes;
const isSelected = context?.selectedTab === id;

const classes = classnames( 'wp-block-woocommerce-product-tab__content', {
Expand All @@ -30,7 +30,7 @@ export function Edit( {

return (
<div { ...blockProps }>
<TabButton id={ id } selected={ isSelected }>
<TabButton id={ id } selected={ isSelected } order={ order }>
{ title }
</TabButton>
<div
Expand Down
35 changes: 24 additions & 11 deletions packages/js/product-editor/src/blocks/tab/tab-button.tsx
View file
Open in desktop
Expand Up @@ -3,24 +3,35 @@
*/
import { Button, Fill } from '@wordpress/components';
import classnames from 'classnames';
import { createElement } from '@wordpress/element';
import { createElement, Fragment } from '@wordpress/element';

/**
* Internal dependencies
*/
import { TABS_SLOT_NAME } from '../../components/tabs/constants';
import { TabsFillProps } from '../../components/tabs';

export const DEFAULT_TAB_ORDER = 100;

const OrderedWrapper = ( {
children,
}: {
order: number;
children: JSX.Element | JSX.Element[];
} ) => <>{ children }</>;

export function TabButton( {
children,
className,
id,
order = DEFAULT_TAB_ORDER,
selected = false,
}: {
children: string | JSX.Element;
className?: string;
id: string;
selected?: boolean;
order?: number;
} ) {
const classes = classnames(
'wp-block-woocommerce-product-tab__button',
Expand All @@ -33,16 +44,18 @@ export function TabButton( {
{ ( fillProps: TabsFillProps ) => {
const { onClick } = fillProps;
return (
<Button
key={ id }
className={ classes }
onClick={ () => onClick( id ) }
id={ `woocommerce-product-tab__${ id }` }
aria-controls={ `woocommerce-product-tab__${ id }-content` }
aria-selected={ selected }
>
{ children }
</Button>
<OrderedWrapper order={ order }>
<Button
key={ id }
className={ classes }
onClick={ () => onClick( id ) }
id={ `woocommerce-product-tab__${ id }` }
aria-controls={ `woocommerce-product-tab__${ id }-content` }
aria-selected={ selected }
>
{ children }
</Button>
</OrderedWrapper>
);
} }
</Fill>
Expand Down
26 changes: 11 additions & 15 deletions packages/js/product-editor/src/components/tabs/tabs.tsx
View file
Open in desktop
@@ -1,12 +1,7 @@
/**
* External dependencies
*/
import {
createElement,
Fragment,
useEffect,
useState,
} from '@wordpress/element';
import { createElement, useEffect, useState } from '@wordpress/element';
import { ReactElement } from 'react';
import { NavigableMenu, Slot } from '@wordpress/components';
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
Expand All @@ -17,6 +12,7 @@ import { navigateTo, getNewPath, getQuery } from '@woocommerce/navigation';
/**
* Internal dependencies
*/
import { sortFillsByOrder } from '../../utils';
import { TABS_SLOT_NAME } from './constants';

type TabsProps = {
Expand All @@ -31,13 +27,6 @@ export function Tabs( { onChange = () => {} }: TabsProps ) {
const [ selected, setSelected ] = useState< string | null >( null );
const query = getQuery() as Record< string, string >;

function onClick( tabId: string ) {
window.document.documentElement.scrollTop = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason we removed the scroll to top on navigation? I'm no longer being scrolled to the top when navigating to a new tab.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this line since I noticed that the navigateTo() utility actually includes that line as well, so it was redundant. On looking at this further I'm realizing that it wasn't working to begin with. This is due to the fact that the form scrolling is actually from the overflow: auto on the interface-skeleton body, and not the document.

This is actually all seeming a bit odd, as the way it's setup the global sidebar is actually not fixed, but the interface skeleton container is. It's then creating a scrollable body container where our form lives. This could be changed to make the sidebar fixed (as it is with the previous react and legacy editors), but that would be diverging from the blocks way, I suppose. It's also creating the dual scrollbars on the right-hand side of the screen:

image

We may want to decide first if we want to considering changing anything on a larger scale here to address both of those issues, or work around them. We can't simply use a getScrollContainer() from @wordpress/dom here since it's in a slot-fill, but there are other ways to work around it of course.

Perhaps a follow-up then?

navigateTo( {
url: getNewPath( { tab: tabId } ),
} );
}

useEffect( () => {
onChange( selected );
}, [ selected ] );
Expand Down Expand Up @@ -81,14 +70,21 @@ export function Tabs( { onChange = () => {} }: TabsProps ) {
<Slot
fillProps={
{
onClick,
onClick: ( tabId ) =>
navigateTo( {
url: getNewPath( { tab: tabId } ),
} ),
} as TabsFillProps
}
name={ TABS_SLOT_NAME }
>
{ ( fills ) => {
if ( ! sortFillsByOrder ) {
return null;
}

maybeSetSelected( fills );
return <>{ fills }</>;
return sortFillsByOrder( fills );
} }
</Slot>
</NavigableMenu>
Expand Down
4 changes: 4 additions & 0 deletions plugins/woocommerce/changelog/fix-tabs-reordering-38002
View file
Open in desktop
@@ -0,0 +1,4 @@
Significance: minor
Type: add

Adding order attributes to product tabs template.
4 changes: 4 additions & 0 deletions plugins/woocommerce/includes/class-wc-post-types.php
View file
Open in desktop
Expand Up @@ -371,6 +371,7 @@ public static function register_post_types() {
array(
'id' => 'general',
'title' => __( 'General', 'woocommerce' ),
'order' => 10,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noting that I think this is a good short-term fix, but long-term I'd love to see this order property removed since this array already determines order and may conflict with the number provided here.

Do we have another issue already to track this long-term and provide a fix for the underlying cause?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joshuatf Originally I believe both @louwie17 and I were considering this a potential future extensibility point, in which case you'd want a way to control the order by using the slot-fill directly. Now that you mention it, though, that wouldn't ultimately make sense since you'd need to use the block extensibility hooks to relay the inner blocks anyways.

If we're considering this exclusive to internal use then I'd agree that the usage of the order is just a temporary measure. Louren's created an issue with GB here, but the only feedback so far doesn't consider it a bug. We may want to weigh in there further.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It actually looks like this may have just been resolved upstream.

Edit: I made a note of this in the future concerns doc.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating the future concerns doc, we should check if that does solve our problem, maybe when Gutenberg 15.8 rolls out? As I am not sure how to best build and run Gutenbergs dev version (although I am sure its relatively straightforward).
But the basic ordering may still be needed as this won't be available until the follow WP version.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tested this with the latest GB trunk version and it does seem to be fixed now 🎉

),
array(
array(
Expand Down Expand Up @@ -491,6 +492,7 @@ public static function register_post_types() {
array(
'id' => 'pricing',
'title' => __( 'Pricing', 'woocommerce' ),
'order' => 20,
),
array(
array(
Expand Down Expand Up @@ -614,6 +616,7 @@ public static function register_post_types() {
array(
'id' => 'inventory',
'title' => __( 'Inventory', 'woocommerce' ),
'order' => 30,
),
array(
array(
Expand Down Expand Up @@ -763,6 +766,7 @@ public static function register_post_types() {
array(
'id' => 'shipping',
'title' => __( 'Shipping', 'woocommerce' ),
'order' => 40,
),
array(
array(
Expand Down