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

fix: Tabs fn should be overrideable #6880

Merged

Conversation

scomea
Copy link
Collaborator

@scomea scomea commented Dec 21, 2023

📖 Description

The protected "setTabs" function in the Tabs component was written as an arrow function which made overriding it difficult as you can't simply call super.setTabs() to invoke the base class function when extending the Tabs class. This change converts that function to a class member so it can be overridden more easily. Additional minor cleanup of component code.

This is a breaking change in that anyone who currently has a workaround to override this function will need to make changes (ie. storing the base class arrow function and invoking it, for example).

🎫 Issues

Ad hoc.

✅ Checklist

General

  • I have included a change request file using $ yarn change
  • I have added tests for my changes.
  • I have tested my changes.
  • I have updated the project documentation to reflect my changes.
  • I have read the CONTRIBUTING documentation and followed the standards for this project.

Component-specific

Copy link
Collaborator

@KingOfTac KingOfTac left a comment

Choose a reason for hiding this comment

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

Thanks Stephane!

Copy link
Collaborator

@bheston bheston left a comment

Choose a reason for hiding this comment

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

It looks like that was the convention for many of the private functions. Is there a benefit to this model in any case? I wonder if we should document it as a coding convention for any particular use.

@chrisdholt
Copy link
Member

It looks like that was the convention for many of the private functions. Is there a benefit to this model in any case? I wonder if we should document it as a coding convention for any particular use.

The intent of "protected" is to enable extensibility for classes which extend this as a base class while maintaining their behavior (especially those which are core to the functionality or something like accessibility). https://www.typescriptlang.org/docs/handbook/2/classes.html#member-visibility

@bheston
Copy link
Collaborator

bheston commented Dec 25, 2023

...intent of "protected"...

I was referring to the arrow function mechanism. I'm not sure what the benefit of that style is as it doesn't support the normal override capability. I was reading it as something you could call from a subclass but not override, like a sort of hybrid private/protected. It seemed intentional though.

@chrisdholt
Copy link
Member

...intent of "protected"...

I was referring to the arrow function mechanism. I'm not sure what the benefit of that style is as it doesn't support the normal override capability. I was reading it as something you could call from a subclass but not override, like a sort of hybrid private/protected. It seemed intentional though.

Gotcha - thx for clarification here! We often use arrows for events, we should probably avoid for protected classes where we do setup like this. I think that’s a good call out.

@chrisdholt chrisdholt merged commit 8023f7e into microsoft:master Jan 4, 2024
5 checks passed
janechu pushed a commit that referenced this pull request Jun 10, 2024
* tabs fn cleanup

* Change files

* template

* add comment

* tweak

* read me

* Update change/@microsoft-fast-foundation-879984ab-c986-4ae7-93f2-bbebf4530962.json

Co-authored-by: Chris Holt <[email protected]>

---------

Co-authored-by: Stephane Comeau <[email protected]>
Co-authored-by: Chris Holt <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants