Update Dependencies #178
Update Dependencies #178
Conversation
🦋 Changeset detectedLatest commit: 64fb464 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
| export const Default: Story = { | ||
| args: { | ||
| trigger: <Button>Open Popover</Button>, | ||
| triggerProps: { asChild: true }, |
There was a problem hiding this comment.
We should normalize a pattern for this across the board at some point. A lot of components that use a trigger have different compositional patterns atm.
| { type: "panel", id: "a", content: "A", size: 50 }, | ||
| { type: "resizeTrigger", id: "a:b" }, | ||
| { type: "panel", id: "b", content: "B", size: 50 }, | ||
| { sectionType: "panel", id: "a", content: "A", minSize: 20 }, |
There was a problem hiding this comment.
With the new type inference that was built for Splitter, the interface for ResizeTriggerSection improperly extends SplitterResizeTriggerProps if we use the naming convention of type as that is used for the type property assigned to the button (button, submit, reset, etc).
| {...rest} | ||
| > | ||
| {sections.map((section) => | ||
| match(section) |
There was a problem hiding this comment.
ts-pattern tip: If we match on the entire section, and then derive the arms based on destructured props a few cool things happen:
-
Since it knows
sectionTypecan only be eitherpanelorresizeTriggeryou can see below that theexhaustivepattern still works properly. I thought this was pretty cool. You can increase the scope of this (i.e. match on multiple props in the same arm) and it will still provide type safety if you miss a combination. -
The entire
sectionis passed to the return handler and its properties are narrowed in terms of type inference! Which is why we don't need to do any casting for i.e.idin theresizeTriggermatch arm, because it is inferred already based on the matchingsectionType.
Cool stuff!
| * Icon. | ||
| */ | ||
| export const Icon = ({ src, ...rest }: IconProps) => { | ||
| const Component = styled(src, icon); |
There was a problem hiding this comment.
Caused linting error: Error: Can't create components during render
Essentially each time Icon rendered, React exectues the function body again, which means styled(src, icon) creates a new styled component on every render. The updated logic uses a neutral container element as a generic wrapper and then uses the dynamic src to render the proper html element at runtime.
| useEffect(() => { | ||
| // attach listener to window `resize` event | ||
| window.addEventListener("resize", handleResize); | ||
| handleResize(); |
There was a problem hiding this comment.
This, as well as the setBreakpoint call within the effect caused linting errors: Calling setState synchronously within an effect body causes cascading renders that can hurt performance.
With the update, we initialize window dimensions once on mount, and then attach proper listeners. Then, instead of running an effect to determine breakpoint state we derive state during render. Because breakpoint is derived from windowDimensions.width and is not independent state, we can compute it directly on each (re)render.
| (props: PropsWithoutRef<ComponentProps<T>>, ref) => { | ||
| const slotStyles = useContext(StyleContext); | ||
|
|
||
| // eslint-disable-next-line react-hooks/refs |
There was a problem hiding this comment.
This is the only line added to this file, the rest was formatting. Happy to dig into this, just didnt want to mess with this file at all as it is used throughout every component. If there is indeed issue here though with this generalized context helper as a whole, it could provide major perf impact in downstream apps, so may be worth digging into in the future!
Description
The main purpose of this PR is updating dependencies (minus storybook related deps) as well as peer dependencies that downstream apps must rely on. A few other changes were made in order to resolve some underlying issues:
itemprop initemPropsonComboboxas it is handled internallySplitterprops and logic to handle breaking changes from the dependency updates as well as provide better type safetyIconto prevent recreation of component classes on every renderuseBreakpointto derive state during render and prevent cascading renders due to synchronoussetStates within effectsTest Steps