-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
feat(compose): add slots and mapPropsToSlotProps #12858
Conversation
-fixed screener
Co-Authored-By: Oleksandr Fediashov <olfedias@microsoft.com>
# Conflicts: # packages/fluentui/react-northstar/src/components/Attachment/AttachmentAction.tsx # packages/fluentui/react-northstar/src/components/Attachment/AttachmentBody.tsx # packages/fluentui/react-northstar/src/components/Attachment/AttachmentDescription.tsx # packages/fluentui/react-northstar/src/components/Attachment/AttachmentIcon.tsx # packages/fluentui/react-northstar/src/components/Box/Box.tsx # packages/fluentui/react-northstar/src/components/Button/Button.tsx # packages/fluentui/react-northstar/src/components/Button/ButtonContent.tsx # packages/react-compose/etc/react-compose.api.md # packages/react-compose/src/compose.ts # packages/react-compose/src/types.ts
…microsoft/fluentui into feat/compose-sig
# Conflicts: # packages/fluentui/react-northstar/test/specs/utils/factories-test.tsx
# Conflicts: # packages/fluentui/react-northstar/test/specs/utils/factories-test.tsx
# Conflicts: # packages/fluentui/react-northstar/src/components/Button/Button.tsx
Co-Authored-By: Roman Sudarikov <pompomon@users.noreply.github.com>
….tsx Co-Authored-By: Roman Sudarikov <pompomon@users.noreply.github.com>
Perf AnalysisNo significant results to display. All results
Perf Analysis (Fluent)Potential regressions comparing to master
Perf comparison
Perf tests with no regressions
|
return slotPropsChain.reduce( | ||
(acc, slotProps) => ({ | ||
...acc, | ||
...(typeof slotProps[slot] === 'function' ? slotProps[slot](props) : slotProps[slot] || {}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this is not a part of compose
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean the whole function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean we should have as part of composeOption
the getSlotProps
function, so we can just use composeOptions.getSlotProps('content')
for example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. Or even composeOptions.slotProps.content
as I don't understand why we need getSlotProps()
as function: to component we pass resolved options already
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not have the props inside compose, that is the reason why I initially made it a separate hook. If we return some function from compose, I think we will need to still invoke it with the properties inside the component. Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a different as in useStyles
we are using mapPropsToStyles
in useStyles
options. But here as functions in chain are available at composeOptions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok let me try to see if everything will work as expected then... I am going to update slotProps
to be mapPropsToSlots: (props) => ({ content: {} })
and try to remove the useSlotProps
hook
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to conclude, after offline discussions we decided to change API of compose to mapPropsToSlots: (props) => ({ content: {} })
, but still use a hook for getting the slot props, as for some components we will need both props + state for resolving the slot props.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My preference would be in compose
: keep slots
and slotProps
as names. Either of these could be and static object map, or a function taking in state
and returning the object map. It keeps the consumer experience super clean:
compose(Button, {
slots: {
icon: MyContainer
},
slotProps: {
icon: { data-is-foo: 'bar'
}
}
We should consider moving mapPropsToStyles
to something maybe higher level to be layered on demand: it is a css-in-js ism specifically for caching so as we move to static
I think a function like mergeProps
would be responsible for deriving the final slots
and slotProps
to be distributed. It would need to be called after state has been reconciled. It could manage resolving things like slots/slotProps and classes functions which all should take in state to resolve their stuff.
This means that compose simply glues together the options and passes them along, while the mergeProps helper could finalize the results for consumption in the template.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have mapPropsToStyleProps
, that's why I decided in the end to go with mapPropsToSlotPros
. If we decide to change the API of the compose we can rename those easily to just stylesProps
and slotProps
, but for now I would rather keep the names similar...
packages/fluentui/react-northstar/src/components/Button/Button.tsx
Outdated
Show resolved
Hide resolved
packages/fluentui/react-northstar/src/components/Button/Button.tsx
Outdated
Show resolved
Hide resolved
Why have you decided to go with
just asking |
@layershifter actually this looks much closer to the other mappings in compose already.. 👍 |
🎉 Handy links: |
This PR introduced the
slots
andmapSlotsToPropSlots
concepts in compose.Those can be used in the following manner:
Or when composing existing component
For now I did not include the root in the slots, but we may easily add it :)