TabPanel: Refactor unit tests in prep for controlled component updates

[aaronrobertshaw]

Related:

• TabPanel: add tabName prop (controlled component) #46704
• Replace Post/Block tab switcher with TabPanel #43414
• Block inspector: tabs slot fill should appear below post/block buttons in DOM #47623

What?

• Refactors TabPanel unit tests in preparation for updating offering TabPanel as a controlled component.

Why?

• Ensuring we have proper test coverage of the current TabPanel will certify current behaviour and provide greater confidence when updating the component.
• Refactoring the TabPanel unit tests to use describe.each will make it easier to test the proposed controlled version of this component

How?

• Attempts to find the middle ground between the new tests proposed within #46704 and current preferred approach to testing controlled/uncontrolled components
• Rather than a straight copy of the tests from #46704, this PR tweaks those tests so they match and pass with the component's current behaviour

Testing Instructions

1. Run npm run test:unit:watch packages/components/src/tab-panel

[aaronrobertshaw]

Note: This PR is only a draft at this point as I'm a little short on time. If it isn't useful as a base to verify the current component on trunk before continuing #46704, please feel free to close it.

There are some differences at this stage between the tests in this PR and those in #46704. These mostly revolve around the fact that it appears the proposed updates prevent onSelect on initial render. I've also tried to match the tests more towards where we might be going rather than the existing tests.

I'll be AFK until late next week but I will try and keep an eye on this.

If this PR helps at all please feel free to pick it up and push any desired changes or additional tests. Of course if I've gone down the wrong track with it, I'll happily close it.

cc/ @ciampo, @madhusudhand

[github-actions]

Flaky tests detected in 4f919b4.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4262407876
📝 Reported issues:

• [Flaky Test] results in two remaining blocks and positions the caret at the end of the second block #39805 in specs/editor/various/block-deletion.test.js
• [Flaky Test] should paste list contents into paragraph #46718 in specs/editor/various/rich-text.test.js

[ciampo]

Thanks Aaron — this PR is definitely on the right track!

I gave it quick look, and I started leaving some notes about what other tests I'd like to add (under the shape of either code comments, or failing unit tests).

If I get to it, I'll try to iterate and add those missing unit tests in the next days.

These mostly revolve around the fact that it appears the proposed updates prevent onSelect on initial render.

This is interesting, and something to consider for the changes in #46704

[ciampo]

@aaronrobertshaw I should have added unit tests for all scenarios that came to mind.

Would you be able to give this PR a good look?

• See if there's any tests that you would add
• See if current tests make sense (and are testing expected behaviour)

I also added a small improvement to the Storybook examples that I created while trying to understand tooltip behaviour when icons are defined for tabs.

[aaronrobertshaw]

Would you be able to give this PR a good look?

Thanks for all the work on this PR while I was AFK, I appreciate it 🙇

While checking out the new tabs with icons and tooltips story, I notice I couldn't navigate with the right arrow key. It appears that the tooltip is the culprit as its popover is being rendered inline. In the editor, I believe the tooltip popovers are rendered via a SlotFill and avoid this issue.

Bug Demo

Screen.Recording.2023-02-24.at.1.02.28.pm.mp4

I tweaked the story to provide a Popover.Slot within a wrapping SlotFillProvider (62e88f7).

We've done similar things with other components such as the ColorPalette but I'm not sure how we best communicate the potential issue or fix it altogether for the TabPanel component. Any suggestions?

• See if there's any tests that you would add
• See if current tests make sense (and are testing expected behaviour)

Other than potentially covering the keyboard navigation when tabs have tooltips, nothing else sprang to mind. The correct tests all made sense to me and ran successfully. Nice work.

[ciampo]

Thank you for spotting that, @aaronrobertshaw !

I went ahead and applied your suggestions — testing keyboard interaction with tooltips, and using a dedicated Popover.Slot when doing so to avoid breakages.

I think this PR is good to go, and so I'm going to approve it (since you're the author), but I'll wait for your confirmation before merging.

We've done similar things with other components such as the ColorPalette but I'm not sure how we best communicate the potential issue or fix it altogether for the TabPanel component. Any suggestions?

I haven't looked too deep into it, but definitely something to keep an eye on — I wonder if the upcoming work on Tooltip will introduce any changes on this behaviour.

[madhusudhand]

Thanks @aaronrobertshaw, @ciampo.
Tests are neatly structured and they cover all the scenarios of uncontrolled mode.

[aaronrobertshaw]

Thanks for the tweaks and extra test covering the keyboard navigation and tooltips @ciampo 👍

I wonder if #48222 will introduce any changes on this behaviour.

I'm not certain. I'd probably need to see how we end up implementing the new Tooltip component.

The underlying Ariakit component can render the Tooltip via a portal, which I think we'd want in regard to the TabPanel. How the Ariakit Portal component works with or can be replaced by our SlotFills is where I'd need to dig deeper. My guess is either way we'd need a wrapper of sorts around the storybook example.

but I'll wait for your confirmation before merging.

The tweaks and new test look good. All unit tests are passing and the component behaves as expected in the Storybook and editors.

Tests are neatly structured and they cover all the scenarios of uncontrolled mode.

Looks like we're in a good position to move forward with offering TabPanel in a controlled mode. I'll go ahead and merge this one.