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

Use React hooks for TabPanel component #22906

Merged
merged 3 commits into from
Jun 5, 2020
Merged

Use React hooks for TabPanel component #22906

merged 3 commits into from
Jun 5, 2020

Conversation

pkvillanueva
Copy link
Contributor

Description

Related to #22890

How has this been tested?

The TabPanel component is used in two places:

  • View switcher for Blocks and Patterns.
  • Experimental Navigation's page navigation.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@ZebulanStanphill ZebulanStanphill added [Package] Components /packages/components [Type] Code Quality Issues or PRs that relate to code quality labels Jun 4, 2020
@youknowriad youknowriad added the Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code label Jun 4, 2020
@ItsJonQ
Copy link

ItsJonQ commented Jun 4, 2020

@pkvillanueva Thank you so much for contributing 🎉 !

I tested this locally, and it looks like it's working as expected 🙌

@ItsJonQ
Copy link

ItsJonQ commented Jun 4, 2020

@pkvillanueva I tested it out again. Looks like it's working :)

I took a closer look at the HTML, and the generated ID looked strange (screenshot below). It started with a number 🤔 . This isn't your fault! It must have always been like this 🤦‍♀️ .

Screen Shot 2020-06-04 at 3 03 42 PM

I checked the HTML specs. Ids must start with a letter.

We updated useInstanceId() recently, allowing for prefixes.
If we update it to something like this:

const instanceId = useInstanceId( TabPanel, 'tab-panel' );

Then the generated ids should look like tab-panel-1-...

That feels much better to me 😊

packages/components/src/tab-panel/index.js Outdated Show resolved Hide resolved
packages/components/src/tab-panel/index.js Outdated Show resolved Hide resolved
@pkvillanueva
Copy link
Contributor Author

@ItsJonQ You are right. I noticed that some other components use this hook without prefix.

Copy link
Member

@ZebulanStanphill ZebulanStanphill left a comment

Choose a reason for hiding this comment

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

Looks good to merge. There are some failing end-to-end tests, but they appear to just be intermittent failures. I can't seem to restart the tests. Try rebasing the branch, which should trigger the automated tests to run again.

@ZebulanStanphill ZebulanStanphill merged commit 4bfcda9 into WordPress:master Jun 5, 2020
@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Jun 5, 2020
@github-actions
Copy link

github-actions bot commented Jun 5, 2020

Congratulations on your first merged pull request, @pkvillanueva! We'd like to credit you for your contribution in the post announcing the next WordPress release, but we can't find a WordPress.org profile associated with your GitHub account. When you have a moment, visit the following URL and click "link your GitHub account" under "GitHub Username" to link your accounts:

https://profiles.wordpress.org/me/profile/edit/

And if you don't have a WordPress.org account, you can create one on this page:

https://login.wordpress.org/register

Kudos!

@github-actions github-actions bot added this to the Gutenberg 8.3 milestone Jun 5, 2020
@outcomer
Copy link

outcomer commented Aug 16, 2020

@pkvillanueva @ZebulanStanphill Hey! Now that you moved component to use useEffect - tab's content render AFTER componentDidUpdate in my block. And i lost the ability to do my stuff after tabs content rendered. Yes i get event componentDidUpdate but it occures after tab panel rendered, but tabs content updates after this event due to using React hook and only once - after getting focus by block, because once tab get status Active - no props changes. I have spent two days to find solution but i see - there are no one, cause this is React logic. And i have to ask you - what to do now? I need full control on my block - once tab's content rendered i need to bind them some events listner.
Let me explain scenario: you have dynamic block that have Preview option. This block in Edit mode show to user select box of type multiple. I want this box to serve with Select2 (cause WP Components does not provide looking good component for select multiple or i just want to do it). So i do init Select2 on tabs content once component mounted, next i click Preview button in block control that do server side render, next click Edit, componentDidUpdate fire after tab pannel rendered and i have to init Select2 here but there no content yet cause it will be provided by useEffect on next step. That is all - i have 0 chances to do what i need.
I am not in WP team, i am theme&plugin developer. So this is the feedback to you from the other side. I have been developing theme ~6 month and also written 12 blocks. Everything worked fine and was debugged and voila - WP 5.5 with React hooks in Gutenberg. And there are no warning anywhere, no explanaition. But the main thing is not clear - why? Gutenberg already has terrible documentation. It is very difficult to understand anything about how it works. And in this situation you confuse everything even more.

@ZebulanStanphill
Copy link
Member

@outcomer It's really hard to understand your issue without actually seeing your code, but it sounds to me like whatever you were doing before was implemented in a rather hacky/fragile way that relied too much on specific implementation details, and therefore was liable to break at any time. I suspect there's a better way to do what you want that would work both before and after the changes in this PR, but again, without seeing your code it's very difficult to know for sure. Unfortunately, I found your textual explanation very difficult to follow.

Perhaps you should ask for help in the Developing with WordPress Forum or StackOverflow. (And make sure to share your code there, so people can see what you're trying to do.) If you find that there's no non-hacky way to do what you want, then open an issue here specifying what kind of extensibility or new APIs you'd like to see introduced.

But at least right now, it looks to me like this pull request has not changed anything in the public API of the TabPanel component, so there's nothing for me (or anyone else involved with this PR) to fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code [Package] Components /packages/components [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants