Adding basic ordering to tab slot fill #38081

joelclimbsthings · 2023-05-02T21:22:41Z

Submission Review Guidelines:

I have followed the WooCommerce Contributing Guidelines and the WordPress Coding Standards.
I have checked to ensure there aren't other open Pull Requests for the same update/change.
I have reviewed my code for security best practices.
Following the above guidelines will result in quick merges and clear and detailed feedback when appropriate.

Changes proposed in this Pull Request:

Adding order attribute to tab blocks, and preventing reordering while rendering.

Closes #38002 .

How to test the changes in this Pull Request:

Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:

Checkout branch and enable product-block-editor feature flag (can do with WCA Test Helper plugin)
Navigate to Products -> Add new to see product editor.
Refresh the page and observe the header tabs as the page loads, and ensure that they don't reorder as they were originally (see issue for video)

github-actions · 2023-05-02T21:29:48Z

Hi @joshuatf,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

github-actions · 2023-05-02T21:48:42Z

Test Results Summary

Commit SHA: a082ef4

Test 🧪	Passed ✅	Failed 🚨	Broken 🚧	Skipped ⏭️	Unknown ❔	Total 📊	Duration ⏱️
API Tests	267	0	0	2	0	269	1m 9s
E2E Tests	187	0	0	10	0	197	20m 46s

To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

mdperez86

After testing this I notice we lost the default tab selected by default when clicking the left menu item or navigating directly from the url.

Screen.Recording.2023-05-03.at.12.39.17.PM.mov

joshuatf

Experienced the same issue as @mdperez86 and left a couple additional comments.

joshuatf · 2023-05-03T16:55:06Z

plugins/woocommerce/includes/class-wc-post-types.php

@@ -371,6 +371,7 @@ public static function register_post_types() {
 							array(
 								'id'    => 'general',
 								'title' => __( 'General', 'woocommerce' ),
+								'order' => 10,


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?

@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.

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

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

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.

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

joshuatf · 2023-05-03T16:57:01Z

packages/js/product-editor/src/components/tabs/tabs.tsx

@@ -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;


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.

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:

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?

joelclimbsthings · 2023-05-03T20:29:02Z

Thanks @mdperez86 and @joshuatf for the reviews. I've fixed the issue with the tab not being selected on load in a082ef4, although the other comments are something of a can of worms. We might consider a follow-up for those.

mdperez86

LGTM @joelclimbsthings nice job!!!

joelclimbsthings added 2 commits May 2, 2023 14:20

Adding basic ordering to tab slot fill

95c05e0

Adding changelog

2c17cd2

github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label May 2, 2023

Adding plugin changelog

ee08a26

joelclimbsthings force-pushed the fix/tabs-reordering-38002 branch from abd9c6f to ee08a26 Compare May 2, 2023 21:25

joelclimbsthings requested a review from a team May 2, 2023 21:29

joelclimbsthings self-assigned this May 2, 2023

joelclimbsthings marked this pull request as ready for review May 2, 2023 21:29

Fixing linting

b36dddf

mdperez86 requested changes May 3, 2023

View reviewed changes

joshuatf reviewed May 3, 2023

View reviewed changes

Fixing issue with initial tab not being selected

a082ef4

joelclimbsthings requested review from joshuatf and mdperez86 May 4, 2023 15:28

mdperez86 approved these changes May 4, 2023

View reviewed changes

joelclimbsthings merged commit 4729ea0 into trunk May 4, 2023
19 checks passed

joelclimbsthings deleted the fix/tabs-reordering-38002 branch May 4, 2023 17:57

github-actions bot added this to the 7.8.0 milestone May 4, 2023

joshuatf mentioned this pull request Jun 9, 2023

POC: Product template extensibility API #38657

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding basic ordering to tab slot fill #38081

Adding basic ordering to tab slot fill #38081

joelclimbsthings commented May 2, 2023 •

edited

github-actions bot commented May 2, 2023 •

edited

github-actions bot commented May 2, 2023 •

edited

mdperez86 left a comment

joshuatf left a comment

joshuatf May 3, 2023

joelclimbsthings May 3, 2023 •

edited

joelclimbsthings May 4, 2023 •

edited

louwie17 May 4, 2023

louwie17 May 5, 2023

joshuatf May 3, 2023

joelclimbsthings May 3, 2023

joelclimbsthings commented May 3, 2023 •

edited

mdperez86 left a comment

Adding basic ordering to tab slot fill #38081

Adding basic ordering to tab slot fill #38081

Conversation

joelclimbsthings commented May 2, 2023 • edited

Submission Review Guidelines:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

github-actions bot commented May 2, 2023 • edited

github-actions bot commented May 2, 2023 • edited

Test Results Summary

mdperez86 left a comment

Choose a reason for hiding this comment

joshuatf left a comment

Choose a reason for hiding this comment

joshuatf May 3, 2023

Choose a reason for hiding this comment

joelclimbsthings May 3, 2023 • edited

Choose a reason for hiding this comment

joelclimbsthings May 4, 2023 • edited

Choose a reason for hiding this comment

louwie17 May 4, 2023

Choose a reason for hiding this comment

louwie17 May 5, 2023

Choose a reason for hiding this comment

joshuatf May 3, 2023

Choose a reason for hiding this comment

joelclimbsthings May 3, 2023

Choose a reason for hiding this comment

joelclimbsthings commented May 3, 2023 • edited

mdperez86 left a comment

Choose a reason for hiding this comment

joelclimbsthings commented May 2, 2023 •

edited

github-actions bot commented May 2, 2023 •

edited

github-actions bot commented May 2, 2023 •

edited

joelclimbsthings May 3, 2023 •

edited

joelclimbsthings May 4, 2023 •

edited

joelclimbsthings commented May 3, 2023 •

edited