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

feat(radar): add onClick handler #2601

Merged
merged 7 commits into from
Jul 5, 2024
Merged

Conversation

Nino3103
Copy link
Contributor

Radar Chart : add onClick handler

  • added onClick props in Radar props
  • adjusted the types to accept onClick in Radar props
  • added a story in Radar storybook with onClick

Issue : #1003

Video

Radar.-.With.On.Click.Storybook.-.31.May.2024.mp4

Copy link

vercel bot commented May 31, 2024

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

Name Status Preview Comments Updated (UTC)
nivo ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 5, 2024 7:43am

@charles-glanceable
Copy link

We are really looking forward to this new feature! Thank you 👍

@plouc plouc added the radar @nivo/radar package label Jun 26, 2024
Copy link
Owner

@plouc plouc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, LGTM, thank you.

However the property should be added to the website (I'm not sure we need a story), also it would be good to add some tests for this new feature.

@Nino3103
Copy link
Contributor Author

Nino3103 commented Jul 4, 2024

Thank you for your answer, I will add it to the website and add some tests 👍 Should I remove the story ?

@Nino3103
Copy link
Contributor Author

Nino3103 commented Jul 4, 2024

Hello @plouc , I added a radar test and updated the website

Radar.chart._.nivo.-.4.July.2024.mp4

packages/radar/tests/Radar.test.tsx Outdated Show resolved Hide resolved
storybook/stories/radar/Radar.stories.tsx Outdated Show resolved Hide resolved
@plouc
Copy link
Owner

plouc commented Jul 5, 2024

@Nino3103, could you please re-sync with master? I had to upgrade a github action, that's why tests are failing.

@Nino3103
Copy link
Contributor Author

Nino3103 commented Jul 5, 2024

@plouc it's resynced with master, tests are passing now 👍

@plouc plouc merged commit 620b099 into plouc:master Jul 5, 2024
4 of 5 checks passed
@plouc
Copy link
Owner

plouc commented Jul 5, 2024

@Nino3103, thank you for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
radar @nivo/radar package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants