Skip to content
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

Fix victory-native/victory-zoom-container clipping bug #3026

Merged
merged 5 commits into from
Jan 14, 2025

Conversation

djm158
Copy link
Contributor

@djm158 djm158 commented Jan 9, 2025

Description

Fixes #3022

Issue summary:

Previously, victory-native/victory-zoom-container spread the result of useVictoryZoomContainer into VictoryContainer

const props = useVictoryZoomContainer(...)
return <VictoryContainer {...props} />;

Where the return type of useVictoryZoomContainer is

{
  props: VictoryZoomContainerMutatedProps;
  children: React.ReactElement[];
}

Because VictoryContainer received the entire props object as a prop, instead of spreading its properties, values such as width and height were undefined - leading to the svg element to render with undefined values in its viewBox, e.g.

<svg viewBox="0 0 undefined undefined" ...>

thus clipping the rendered chart.

Solution:

Destructure the props/children result from useVictoryContainer and pass correctly to VictoryContainer

Additional changes:

  • This patch also adds the following dependencies
    • react-native-web
    • @expo/metro-runtime
      to the demo/rn project to enable web browser debugging.
  • Update victory-native README to use pnpm instead of yarn and include build --watch step for improved debugging

Type of Change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Ran rn/demo project, added victory-zoom-container to confirm before/after changes visually.

Screenshots

Before

Screenshot 2025-01-09 at 11 24 35 AM

After

Screenshot 2025-01-09 at 11 21 45 AM

Checklist: (Feel free to delete this section upon completion)

  • I have included a changeset if this change will require a version change to one of the packages.
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have run all builds, tests, and linting and all checks pass
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

djm158 added 5 commits January 9, 2025 10:59
Issue summary:

Previously, victory-native/victory-zoom-container spread the result of
`useVictoryZoomContainer` into `VictoryContainer`

```ts
const props = useVictoryZoomContainer(...)
return <VictoryContainer {...props} />;
```

Where the return type of `useVictoryZoomContainer` is

```
{
  props: VictoryZoomContainerMutatedProps;
  children: React.ReactElement[];
}
```

Because `VictoryContainer` received the entire `props` object as a prop, instead of spreading its
properties, values such as `width` and `height` were `undefined` - leading to the viewbox for the svg
element to render as `<svg viewBox="0 0 undefined undefined" ...>` thus clipping the rendered chart.

Solution:

Destructure the props/children result from `useVictoryContainer` and pass correctly to
`VictoryContainer`j
Copy link

changeset-bot bot commented Jan 9, 2025

🦋 Changeset detected

Latest commit: f3e26d3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 30 packages
Name Type
victory-native Patch
victory-area Patch
victory-axis Patch
victory-bar Patch
victory-box-plot Patch
victory-brush-container Patch
victory-brush-line Patch
victory-candlestick Patch
victory-canvas Patch
victory-chart Patch
victory-core Patch
victory-create-container Patch
victory-cursor-container Patch
victory-errorbar Patch
victory-group Patch
victory-histogram Patch
victory-legend Patch
victory-line Patch
victory-pie Patch
victory-polar-axis Patch
victory-scatter Patch
victory-selection-container Patch
victory-shared-events Patch
victory-stack Patch
victory-tooltip Patch
victory-vendor Patch
victory-voronoi-container Patch
victory-voronoi Patch
victory-zoom-container Patch
victory Patch

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

Copy link

vercel bot commented Jan 9, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
victory ✅ Ready (Inspect) Visit Preview Jan 9, 2025 4:44pm

@djm158 djm158 changed the title Fix/3022 zoom container Fix victory-native/victory-zoom-container clipping bug Jan 9, 2025
@djm158 djm158 self-assigned this Jan 9, 2025
@zibs zibs self-requested a review January 9, 2025 21:26
packages/victory-native/README.md Show resolved Hide resolved
@djm158 djm158 merged commit 1c8217d into main Jan 14, 2025
7 checks passed
@djm158 djm158 deleted the fix/3022-zoom-container branch January 14, 2025 16:42
@fmd-ci fmd-ci mentioned this pull request Jan 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

VictoryZoomContainer broken since 37.1.0
2 participants