digdir / designsystemet

Designsystemet
https://designsystemet.no
MIT License
81 stars 37 forks source link

Polymorphism support for our React components #1124

Closed mimarz closed 8 months ago

mimarz commented 11 months ago

Our implementation of polymorphism with as prop using OverridableComponent has its drawback with callback functions not being updated to use the casted type which has led to a few bumps when using this for our components and wanted feature.

Suggest we investigate the following to figure out what to do:

Adopting asChild approach could fix this issue but it would enforce other contraints on our users components when it comes to composition.

Alternatively we can use Box to test using asChild for polymorphism.

### Tasks
- [ ] https://github.com/digdir/designsystem/issues/1282
- [ ] https://github.com/digdir/designsystem/issues/1283
- [ ] https://github.com/digdir/designsystem/issues/1285
- [ ] https://github.com/digdir/designsystem/issues/1296
- [ ] https://github.com/digdir/designsystem/issues/1297
- [ ] https://github.com/digdir/designsystem/issues/1298
- [ ] https://github.com/digdir/designsystem/issues/1305
- [ ] https://github.com/digdir/designsystem/issues/1306
- [ ] https://github.com/digdir/designsystem/issues/1307
- [ ] https://github.com/digdir/designsystem/issues/1308
- [ ] https://github.com/digdir/designsystem/issues/1309
- [ ] https://github.com/digdir/designsystem/issues/1311
- [ ] https://github.com/digdir/designsystem/issues/1310
- [ ] https://github.com/digdir/designsystem/issues/1312
- [ ] https://github.com/digdir/designsystem/issues/1313
- [ ] https://github.com/digdir/designsystem/issues/1314
- [ ] https://github.com/digdir/designsystem/issues/1315
- [ ] https://github.com/digdir/designsystemet/issues/1326
- [ ] https://github.com/digdir/designsystemet/issues/1438
- [ ] https://github.com/digdir/designsystemet/issues/1531
mimarz commented 10 months ago

We decided to test this using asChild with Box using Slot.

mimarz commented 10 months ago

After testing asChild and it working alongside as we prefer asChild as its much more flexible for end users, especially with callbacks having correct types.

We will use @radix-ui/react-slot for asChild feature.

mimarz commented 10 months ago

We hit a snag with using the Slot component for Button and Tabs, in general components where we interspace things with children, such as icon passed via an icon prop. This fails due to Slot needing to have a single children for it to merge with.

We are currently investigating other options such as:

  1. Make use of Mantines more advanced as ( component ) support
  2. Drop use of icon prop and have users handle icons themselves
  3. Separate sub-component (slot-component) for placement of passed icon.
    • Example: #1321
mimarz commented 10 months ago

After some testing and investigation:

After a short meeting on jan 5. we decided to remove the ability to pass icons through properties and instead favour them being passed as children #1333

This issue will be put on hold until we finish #1333 and #1342

Barsnes commented 9 months ago

After some testing and investigation:

  • Chakras/Mantines polymorphic factory are very cumbersome or complex to try to convert to our own.
  • Radix's Slot still feels like a better and more robust solution.
  • Remove the ability to pass icon through icon prop has also the added benefit of providing more flexibility to how users use icons inside our stories.

After a short meeting on jan 5. we decided to remove the ability to pass icons through properties and instead favour them being passed as children #1333

This issue will be put on hold until we finish #1333 and #1342

The two other issues has been completed, should we move forward with this issue?

Barsnes commented 8 months ago

Closing as completed