-
Notifications
You must be signed in to change notification settings - Fork 88
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: Normalize component structure and exports #2728
Conversation
- ensure all components have an exported prop type - normalize `Base<Component>Props` as type holding custom props - normalize `<ComponentType>Props` as type union of `Base<Component>Props` (if exists) and wrapper element props type - export misc types and helper functions
Moving to draft as I also include forwardRef support and fix some components I missed. |
Finished first pass. There are some tough problems to solve with existing forwardRef components with limited scope, container components that would leave internal input child without a ref path, and inconsistent prop patterns of child component props expected in container prop root or in subproperty. |
- onChange property on Accordion exposed to emit currently open items - AccordionItem broken down into individual components - Relevant tests from Accordion moved to individual component tests - Stories/tests get test data from test-fixtures - All components are forwardRef - AccordionItem content marked as deprecated in favor of children - New props exposing refs to composed children - Misc enhancements
- Broken into modular components with tests - Relevant tests moved to subcomponents - Ref props to subcomponents from Alert added
- add Base components focused on just their root element - Accordion/AccordionItem/Alert components handle all child component compositing using individual subcomponents - Ref props swapped for component props (which includes ref)
- subcomponent breakout - tests - use Accordion subcomponents where possible in Banner subcomponents - use Grid component instead of manual div
I have an almost-complete implementation of forwardRef+asCustom via TooltipTrigger but getting the typescript typing correct has been a roadblock. |
- revert all asCustom components (except tooltip) to originals for now - misc fixes so that linting, tests, and building pass
- sitealert uses `Alert` instead of manual element - headingLevel added to `SiteAlert` - isX booleans - Alert type made optional - sb story titles normalized to uppercase
Hi @jpandersen87 , what are your plans/advice for others going forward in this workstream? |
There's some major hurdles in regards to creating a type-safe pattern that can be applied to all components so that they're created via forwardRef and can forward that ref to the nearest relevant wrapper/child. |
I may retry this later where I at least create forwardRef versions of simpler components so at least there's forward momentum in this goal. |
Summary
Everything noted below is under the pretense of NOT breaking existing usage patterns. All changes are intended to be additions, with JSDoc deprecation comments used where appropriate.
[Component]Base
component that focuses on root element rendering, if applicableBase[Component]Props
as type holding custom props[Component]Props
asReact.ComponentPropsWithRef<typeof [Component]>
[Component]Ref
asReact.ComponentRef<typeof [Component]>
React.ForwardRefRenderFunction
or wherever else appropriate as a reliable aliasReact.[Component]PropsWith[out]Ref
for props that are intended to be directly forwarded to child componentJSX.IntrinsicElements
is type-incompatible)divProps
->headerProps
children
->children
isX
name format for booleans (adding deprecation markers where appropriate)undefined
for falseynessonChange
prop onAccordion
)Related Issues or PRs
#2667
#2666
#2587
#2232
#2599
#2000
How To Test
Screenshots (optional)