Add FocusTrapCallout Component#7488
Conversation
cliffkoh
left a comment
cliffkoh
left a comment
There was a problem hiding this comment.
I'm a little confused by the need to create a standalone FocusTrapCallout which is exported at the index - is there a desire for people to use this in isolation?
If not, could we not just inline the whole thing?
| import { IBaseCardProps } from '../BaseCard.types'; | ||
| import { FocusTrapCallout, ICalloutProps } from '../../Callout'; | ||
|
|
||
| export interface ICardCalloutProps extends IBaseCardProps<{}, {}, {}> { |
There was a problem hiding this comment.
I just realized that whatever I said above about FocusTrapCallout applies to this very component itself too, although at least this is a stateless component (though not explicitly typed as such).
Can we document these props?
cliffkoh
left a comment
cliffkoh
left a comment
There was a problem hiding this comment.
Hey @atneik, I didn't realize CardCallout.tsx was in HoverCard and not within Callout component. As such, please disregard the questions asking if it needed to be exported.
In this case though, we really need to do a lot more work on the documentation front (writing examples, treating FocusTrapCallout as a first class "component" as far as documentation goes).
We should also make sure to add tests/screener since this is meant be to directly used by consumers as well.
|
@cliffkoh Yes it would need to be exported so that consumers and some of our components can use it. I see this one in lines with other composite components such as I totally agree we need to better document these types of components. We do have examples using them, but still need a list of these "modified components" on the component page itself. I think updating the documentation to include them should be out of scope of this PR. |
|
cc @dzearing @aditima @natalieethell @micahgodbolt @Jahnp regarding the need to all-up have a better documentation story for exported component variants. (We will need a component documentation overhaul anyway what with the upcoming component slots feature) |
|
|
Any idea why the bundle size took a hit? |
cc: @aftab-hassan |
The sizes are generated by the script @cliffkoh For now, can we unblock this PR by making the size audit check optional ? |
|
@aftab-hassan the size audit check is already optional.. the PR is blocked because a code owner has not signed off. |
|
🎉 Handy links: |
7 participants
Pull request checklist
$ npm run changeDescription of changes
FocusTrapCalloutto allow ability to trap focus in aCallout. Consumers can pass in the props as they would have in a regularCalloutin addition they can passFocusTrapZoneprops viafocusTrapPropsto customize the trap.CardCalloutin HoverCard to use the above componentMicrosoft Reviewers: Open in CodeFlow