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

Candlestick labels show incorrect midpoint label #2923

Closed
carbonrobot opened this issue Oct 16, 2024 · 5 comments · Fixed by #2931
Closed

Candlestick labels show incorrect midpoint label #2923

carbonrobot opened this issue Oct 16, 2024 · 5 comments · Fixed by #2931
Assignees
Labels
Issue: Accepted The submitted issue has been confirmed by the Victory core team Type: Bug 🐛 Oh no! A bug or unintentional behavior

Comments

@carbonrobot
Copy link
Contributor

When setting the labels property to true, candlestick charts incorrectly show the value true as a midpoint label, but should show no midpoint label.

Image

Example Code
<VictoryChart
  domainPadding={{ x: 25 }}
  theme={VictoryTheme.clean}
>
  <VictoryCandlestick
    labels
    data={[
      {
        x: "3/1/23",
        open: 5,
        close: 10,
        high: 15,
        low: 0,
      },
      {
        x: "3/2/23",
        open: 10,
        close: 15,
        high: 20,
        low: 5,
      },
      {
        x: "3/3/23",
        open: 15,
        close: 20,
        high: 22,
        low: 10,
      },
      {
        x: "3/4/23",
        open: 20,
        close: 10,
        high: 25,
        low: 7,
      },
      {
        x: "3/5/23",
        open: 10,
        close: 8,
        high: 15,
        low: 5,
      },
    ]}
  />
</VictoryChart>
@carbonrobot carbonrobot added Type: Bug 🐛 Oh no! A bug or unintentional behavior Issue: Accepted The submitted issue has been confirmed by the Victory core team labels Oct 16, 2024
@nstolpe
Copy link
Contributor

nstolpe commented Oct 25, 2024

@carbonrobot I have a branch w/ a potential fix for this, but I don't have permissions to push to the repo. Could you grant them when you have a few minutes?

@carbonrobot
Copy link
Contributor Author

Looks good, please run pnpm changeset to add a changeset for a patch release

@nstolpe
Copy link
Contributor

nstolpe commented Oct 28, 2024

Looks good, please run pnpm changeset to add a changeset for a patch release

Added. Should I add a test for this too?

@carbonrobot
Copy link
Contributor Author

That would be awesome, I didn't remember we even had a test setup for this component.

@nstolpe
Copy link
Contributor

nstolpe commented Oct 28, 2024

Yeah, I was just looking around in the folder to see if there were any and found it, so figured it wouldn't hurt to add coverage. The test's in my last commit, so should be good to go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue: Accepted The submitted issue has been confirmed by the Victory core team Type: Bug 🐛 Oh no! A bug or unintentional behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants