Containers and portal refactor

[KenanYusuf]

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): #2799 (comment)

Test results (native): #2799 (comment)

[changeset-bot]

🦋 Changeset detected

Latest commit: b82bc5b

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

[vercel]

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

[KenanYusuf]

Manual testing results

victory-brush-container

Before

brush.container.-.before.mp4

After

brush.container.-.after.mp4

---

victory-cursor-container

Before

cursor.container.-.before.mp4

After

cursor.container.-.after.mp4

---

victory-selection-container

Before

selection.container.-.before.mp4

After

selection.container.-.after.mp4

---

victory-voronoi-container

Before

voronoi.container.-.before.mp4

After

voronoi.container.-.after.mp4

---

victory-zoom-container

Before

zoom.container.-.before.mp4

After

zoom.container.-.after.mp4

---

victory-create-container

Before

create.container.-.before.mp4

After

create.container.-.after.mp4

[KenanYusuf]

Victory Native testing results (v36.9.2-next.3)

Before

before.mp4

After

after.mp4

[Burnett2k]

I was able to get all of the web versions of the components tested and reviewed today. I did not find any major issues.
Tomorrow, I will go through the VictoryNative components as well and give final sign off if it all looks good.
As I tested and tried to learn about each component, I jotted down some notes for improvements, but I'll create those as separate issues so this PR doesn't get bogged down with things you didn't change.
Really good attention to detail on this refactor! It's a ton of code changes and looks to be really well done so far.

[Burnett2k]

I finished a code review of the remaining react-native items and did not find any issues. Wanted to complete my review by testing the native changes locally, but I'm still working on getting that set up on my computer. Watched your testing videos and those give me more confidence that everything is working correctly.

Additionally, I reviewed your responses to my previous comments and they all look good! 🚀

[carbonrobot]

This looks good, but since the size increases 100KB, we need to wait until #2804 is merged before we merge this one.