base-ui
base-ui copied to clipboard
[core] Improve code readability when using mergeProps
A proposal of how we can write more readable code when it comes to using mergeProps. There is one subtle issue with the typings, if I use React.ComponentsPropsWithRef<'div'> on the defaultProps const, it fails because of our refs, as they are always defined as React.useRef<HTMLElement | null>, so I just kept the type as is, but added the event handlers event types. Let me know if you think we should change this.
In addition I don't think we need to always define the new const, maybe just in places where the code is not readable. I changed two examples places, if we agree on the pattern I will do the changes on the rest codebase.
Deploy Preview for base-ui ready!
| Name | Link |
|---|---|
| Latest commit | 8bbe9648d8c11395fbd86b7a6f0da1f7491984bd |
| Latest deploy log | https://app.netlify.com/sites/base-ui/deploys/67c98854bd3d9c0008b5fc0f |
| Deploy Preview | https://deploy-preview-1540--base-ui.netlify.app |
| Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site configuration.
IMO what we have currently is OK and I don't see a problem with readability. The proposed solution introduces an additional useMemo call, that can have a slight perf impact.
Agree with MichaΕ. However there's one thing that this change highlights that I like: if it can be a value (rather than a function), it should be a value. Imho, mergeProps is used too much and some of those components should be returning their props directly rather than a prop getter. The issue is that calling mergeProps multiple times (each time a new set of props need merging) is slower than calling it once (when we have all the props sets to be merged together).
Likely the only time we need to return a function is when anything below uses useButton as merging its props requires access to other external props (i.e. the onKeyDown prop needs to access external onClick). Unfortunately, useButton is pretty widely used .