victory
victory copied to clipboard
FormidableLabs
Containers and portal refactor
Description
This PR is a fairly chunky refactor of the internals of all of the container components. The component APIs should not have changed. The premise of the changes: use component composition and hooks instead of class-based inheritance for the container components.
Refactored packages include:
- victory-container
- victory-portal
- victory-brush-container
- victory-cursor-container
- victory-selection-container
- victory-voronoi-container
- victory-zoom-container
- victory-create-container
- victory-native (the corresponding components for the list above)
victory-container
This component has been completely rewritten to use React hooks and modern JS instead of relying heavily on helper functions.
victory-portal
This component has also been completely rewritten, as the old approach to creating portals did not work with refs created through useRef. The new portal component uses React.createPortal instead of the custom portal solution used previously.
voronoi, selection, brush, cursor and zoom containers
The container components previously overwrote methods of the VictoryContainer class, however, due to this being refactored to a function in this component, the inheritance no longer worked. These components were also refactored to functions, using composition and hooks to pass the container-specific functionality to the VictoryContainer.
Before:
export function selectionContainerMixin<
TBase extends ComponentClass<TProps>,
TProps extends VictorySelectionContainerProps,
>(Base: TBase) {
// @ts-expect-error "TS2545: A mixin class must have a constructor with a single rest parameter of type 'any[]'."
return class VictorySelectionContainer extends Base {
...
// Overrides method in VictoryContainer
getChildren(props: TProps) {
return [...React.Children.toArray(props.children), this.getRect(props)];
}
}
export const VictorySelectionContainer = selectionContainerMixin(VictoryContainer);
After:
export const useVictorySelectionContainer = (
initialProps: VictorySelectionContainerProps,
) => {
...
return {
props, // modified props to pass to VictoryContainer,
children, // modified elements to render as children of VictoryContainer
};
};
export const VictorySelectionContainer = (
initialProps: VictorySelectionContainerProps,
) => {
const { props, children } = useVictorySelectionContainer(initialProps);
return <VictoryContainer {...props}>{children}</VictoryContainer>;
};
The purpose of separating the component and its functionality via the hook is so that the containers can be combined (see victory-create-container changes).
Mutated props
Victory has a lot of prop mutation. The modified props up until now meant that to satisfy TypeScript, we would have to add a non-user facing prop to the user facing prop interface or ignore the type errors. These components now have two interfaces: one for the user facing props (e.g. VictorySelectionContainerProps), the other for props that get added/modified through helper functions (e.g. VictorySelectionContainerMutatedProps).
export interface VictorySelectionContainerProps {
// props that users provide to a component
allowSelection?: boolean;
...
}
interface VictorySelectionContainerMutatedProps
extends VictorySelectionContainerProps {
// props that are added through helper functions
x1: number;
x2: number;
...
}
defaultEvents
The container classes had a static method called defaultEvents which was responsible for assigning event handlers to a specific target (e.g. parent, data) in a chart. Event handling will need to be refactored at some point, however, it was out of scope for this rewrite. The defaultEvents functionality has been maintained by adding defaultEvents method to each of the new function components.
victory-create-container
The createContainer and makeCreateContainerFunction have been totally rewritten so that instead of merging the methods of each container class, they merge the properties and children for each container through their respective hooks.
Testing
To test the changes, I used the demo site to compare the functionality of the latest version of victory (v36.9.1) to the changes in this PR.
Test results (web): https://github.com/FormidableLabs/victory/pull/2799#issuecomment-1952650800
Test results (native): https://github.com/FormidableLabs/victory/pull/2799#issuecomment-1954491502
🦋 Changeset detected
Latest commit: b82bc5b04f7b1b47d96546450560b98f86e41e7e
The changes in this PR will be included in the next version bump.
This PR includes changesets to release 31 packages
| Name | Type |
|---|---|
| victory | Minor |
| victory-brush-container | Minor |
| victory-core | Minor |
| victory-create-container | Minor |
| victory-cursor-container | Minor |
| victory-native | Minor |
| victory-selection-container | Minor |
| victory-tooltip | Minor |
| victory-voronoi-container | Minor |
| victory-zoom-container | Minor |
| victory-area | Minor |
| victory-axis | Minor |
| victory-bar | Minor |
| victory-box-plot | Minor |
| victory-brush-line | Minor |
| victory-candlestick | Minor |
| victory-canvas | Minor |
| victory-chart | Minor |
| victory-errorbar | Minor |
| victory-group | Minor |
| victory-histogram | Minor |
| victory-legend | Minor |
| victory-line | Minor |
| victory-pie | Minor |
| victory-polar-axis | Minor |
| victory-scatter | Minor |
| victory-shared-events | Minor |
| victory-stack | Minor |
| victory-vendor | Minor |
| victory-voronoi | Minor |
| victory-docs | Minor |
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
![changeset-bot[bot] avatar](https://avatars.githubusercontent.com/in/28616?v=4 "Published by a pub.dev verified publisher"){kind=small}
The latest updates on your projects. Learn more about Vercel for Git ↗︎
| Name | Status | Preview | Comments | Updated (UTC) |
|---|---|---|---|---|
| victory | ✅ Ready (Inspect) | Visit Preview | 💬 Add feedback | May 29, 2024 7:06pm |
![vercel[bot] avatar](https://avatars.githubusercontent.com/in/8329?v=4 "Published by a pub.dev verified publisher"){kind=small}
Manual testing results
victory-brush-container
Before
https://github.com/FormidableLabs/victory/assets/9557798/c7269d0f-975a-457d-82b7-fdd0a62ab83f
After
https://github.com/FormidableLabs/victory/assets/9557798/1a089367-9412-456c-a82d-93a0ffc6e3d2
victory-cursor-container
Before
https://github.com/FormidableLabs/victory/assets/9557798/fc833ab8-bcb7-4b1f-9f5b-97d8ce3c56ab
After
https://github.com/FormidableLabs/victory/assets/9557798/e5f208b9-0129-4bd8-9986-474c646deecb
victory-selection-container
Before
https://github.com/FormidableLabs/victory/assets/9557798/85d7389c-0cdd-49d2-ac08-172daf279a90
After
https://github.com/FormidableLabs/victory/assets/9557798/6c7e4cbc-0c23-40fa-9d13-27f55ea3dfe0
victory-voronoi-container
Before
https://github.com/FormidableLabs/victory/assets/9557798/39dc1ffc-bc96-4f07-8e21-6876e0d62fbc
After
https://github.com/FormidableLabs/victory/assets/9557798/ac2d5042-9fbb-405d-af15-867886e91842
victory-zoom-container
Before
https://github.com/FormidableLabs/victory/assets/9557798/554bd619-4cd3-46ee-80fe-f91768f38d68
After
https://github.com/FormidableLabs/victory/assets/9557798/2b613496-1d6d-4d72-b18b-1e1f1703b5df
victory-create-container
Before
https://github.com/FormidableLabs/victory/assets/9557798/770fea4e-4880-409c-b129-93c46ce3c1f5
After
https://github.com/FormidableLabs/victory/assets/9557798/71f6c04f-91f1-4e65-8ff6-74b89b8f0bf0
Victory Native testing results (v36.9.2-next.3)
Before
https://github.com/FormidableLabs/victory/assets/9557798/0fb6b5ec-82d7-4c0d-a23b-b7f008c6a4df
After
https://github.com/FormidableLabs/victory/assets/9557798/0084de11-bc1c-4741-a0f2-ec0989e27ed0
This looks good, but since the size increases 100KB, we need to wait until #2804 is merged before we merge this one.
