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

Add mesh to bump chart #2496

Merged
merged 10 commits into from
Apr 29, 2024
Merged

Add mesh to bump chart #2496

merged 10 commits into from
Apr 29, 2024

Conversation

santiperone
Copy link
Contributor

Hi! At my company we were needing the ability to show point-specific tooltips within a Bump chart. I ended up copying the bump component into our codebase to cover the use case, but thought others might find this useful as well.

The PR basically just adds a mesh layer and triggers the interactivity from the lines to the mesh (while still updating the activeSeries to keep behaviours as is).

Default behaviour for the component should remain the same.

Still not so convinced on the API decisions I made with tooltip and the handlers. And maybe instead of 'tooltipAchor' using useMesh as the line component does would be better.

I'm open to ideas and suggestions on this matter, and I can update the changes once we reach common ground there.

Copy link

vercel bot commented Jan 8, 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 Mar 13, 2024 5:24pm

Copy link

codesandbox-ci bot commented Jan 8, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 2a64455:

Sandbox Source
nivo Configuration

@santiperone
Copy link
Contributor Author

@plouc Is this library still being mantained? I noticed there's a long list of open PRs that aren't getting merged nor reviewed.
I'm interested in contributing to the project on te long term.
Also do you have any channel (e.g. discord) where discussions regarding the library take place?

@plouc
Copy link
Owner

plouc commented Mar 4, 2024

@santiperone, the answer is that it really depends on how much time I can find to work on the project, so while I want to keep the project alive, lately I could only spend a few hours here and there.

@santiperone
Copy link
Contributor Author

santiperone commented Mar 4, 2024

@plouc I can imagine! Again, we're using this a lot and I'd be glad to help if you point me in the right direction.

packages/bump/src/bump/Bump.tsx Outdated Show resolved Hide resolved
packages/bump/src/bump/Bump.tsx Outdated Show resolved Hide resolved
packages/bump/src/bump/Bump.tsx Show resolved Hide resolved
packages/bump/src/bump/Bump.tsx Outdated Show resolved Hide resolved
packages/bump/src/bump/LinesLabels.tsx Outdated Show resolved Hide resolved
const point: BumpPoint<Datum, ExtraProps> = {
...rawPoint,
serie,
isActive: activeSerieIds.includes(serie.id),
isInactive: activeSerieIds.length > 0 && !activeSerieIds.includes(serie.id),
isActive: activePointIds.includes(serie.id),
Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't this check be on the point id rather than the series?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I actually messed up here, i guess I made the changes without thinking too much about it.
I'll rollback this to just check the active serie.
Adding the activePoint opens up a lot of possibilities in terms of styling as well though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Open to ideas here, for now I think it should be fine to just follow the series for the active point styles

packages/bump/src/bump/hooks.ts Outdated Show resolved Hide resolved
packages/bump/src/bump/hooks.ts Outdated Show resolved Hide resolved
packages/bump/src/bump/hooks.ts Outdated Show resolved Hide resolved
packages/bump/src/bump/hooks.ts Outdated Show resolved Hide resolved
@plouc
Copy link
Owner

plouc commented Mar 7, 2024

@plouc I can imagine! Again, we're using this a lot and I'd be glad to help if you point me in the right direction.

@santiperone, I reviewed the PR, regarding the lock file, could you please try to take the one from the master, using the new pnpm version specified in the main package.json file?

@santiperone
Copy link
Contributor Author

@plouc I added some changes addressing your comments, I don't think it's quite ready yet but it's getting closer.
Regarding the lockfile i think trying to keep the one in master is breaking the CI jobs.
Could you take a look at the changes so far?

@plouc
Copy link
Owner

plouc commented Apr 29, 2024

@santiperone, sorry for the wait, and thank you for this addition, I'm going to merge as is, try it locally, and adjust myself, if necessary.

@plouc plouc merged commit 43db298 into plouc:master Apr 29, 2024
4 checks passed
@plouc
Copy link
Owner

plouc commented Apr 30, 2024

@santiperone, I'll handle the adjustments in #2572.

@plouc plouc mentioned this pull request Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants