search

microsoft / fluentui

Fluent UI web represents a collection of utilities, React components, and web components for building web applications.
https://react.fluentui.dev
Other
18.59k stars 2.75k forks source link

Add Component Ref to Facepile Button #8257

Closed tabrumle closed 3 years ago

tabrumle commented 5 years ago

In its current state a Facepile does not allow for passing in a component ref to Facepile buttons. This makes it difficult to do custom behavior such as having a callout appear on the button in different scenarios, or doing custom focus behavior. I'd like to add onto Facepile to allow for passing in refs to the FacepileButtons.

Facepile

KevinTCoughlin commented 5 years ago

@tabrumle, should I add you as an assignee? It sounds like you want to pursue this work which I'd be happy to facilitate getting into master.

I admittedly don't have full history of Facepile, but saw that both Facepile and FacepileButton extend BaseComponent which should resolve componentRef. However, FacepileButton is specifically skipping componentRef resolution currently, see https://github.com/OfficeDev/office-ui-fabric-react/blob/c2c40eecf58899bc7a5d98a866956e617a7c8373/packages/office-ui-fabric-react/src/components/Facepile/FacepileButton.tsx#L11

As an FYI, the Fabric team is working towards removing BaseComponent inheritance, see https://github.com/OfficeDev/office-ui-fabric-react/pull/8169 https://github.com/OfficeDev/office-ui-fabric-react/pull/8184 for examples and rationale.

tabrumle commented 5 years ago

Yes, please do. My current understanding of the issue is that I will need to investigate why we are skipping the component ref resolution. If there is no issue with us undoing this, then we will need a way to pass back a ref to the FacepileButton component from Facepile. To do this I think we could add a prop to IFacepilePersona called componentRef?: IRefObject<IButton>; We could then pull this ref out in Facepile from _getElementProps and add it to the object we are passing back to FacepileButton. FacepileButton will then have this ref in its props and will pass it to its underlying BaseButton component. I think this would bypass any need for BaseComponent, but let me know if you think there would be any issue with this possible solution.

KevinTCoughlin commented 5 years ago

If we could access the inner FacepileButton via an existing interface member which exposes the props that would be ideal. It seems like that might be possible via https://github.com/OfficeDev/office-ui-fabric-react/blob/c2c40eecf58899bc7a5d98a866956e617a7c8373/packages/office-ui-fabric-react/src/components/Facepile/Facepile.types.ts#L37 or similar. Just want to make sure we minimize API surface changes.

It looks like componentRef resolution bypass was added as part of https://github.com/OfficeDev/office-ui-fabric-react/pull/6561/files#diff-929e175f7002ccb72564668b0b838aa4R11 so @kkjeer may have context.

I'm OK with leaving BaseComponent as-is for your work unless it gets in the way.

KevinTCoughlin commented 5 years ago

@tabrumle, I checked-in with @kkjeer regarding disabling componentRef resolution and we unfortunately don't have context any longer.

My advice would be to remove that line and see if anything breaks. We'll review at PR time to be doubly sure if nothing stands-out.

If you need help with implementation or want to punt to me, please let me know. Otherwise here if you need me as you work on a patch!

tabrumle commented 5 years ago

Thanks for the follow up. I'll be sure to reach out with any questions!

KevinTCoughlin commented 5 years ago

@tabrumle any traction here? If not I'd be happy to pick up the work :).

msft-fluent-ui-bot commented 3 years ago

Because this issue has not had activity for over 150 days, we're automatically closing it for house-keeping purposes.

Still require assistance? Please, create a new issue with up-to date details.