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: accept any path element attribute as a prop #4855

Merged
merged 9 commits into from
Dec 4, 2024

Conversation

mhuggins
Copy link
Contributor

@mhuggins mhuggins commented Nov 28, 2024

This is an attempt to implement a solution for #4821.

Unfortunately, I could not seem to get my development environment set up following the contributing instructions. After running pnpm install, I was still seeing lint/typecheck errors before even beginning to make changes.

Copy link

changeset-bot bot commented Nov 28, 2024

🦋 Changeset detected

Latest commit: 944238b

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

This PR includes changesets to release 1 package
Name Type
@xyflow/react 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

@peterkogo
Copy link
Member

@mhuggins Thanks for the initial PR! I saw that we can simplify a couple of things here and there.

I tried passing pathLength to the <BaseEdge /> which now works but Typescript is complaining.

I don't understand why even this is throwing a Typescript error:

const x : React.HTMLAttributes<SVGPathElement> = { pathLength: 5 };

Do you have any idea?

@mhuggins
Copy link
Contributor Author

mhuggins commented Dec 2, 2024

@peterkogo I fixed this by changing HTMLAttributes<...> to SVGAttributes<...> for the typing. I realized there were a couple other places that appeared to use this generic type incorrectly, so I updated those too.

I also added an example that demonstrates the pathLength being used in conjunction with a CSS animation that leverages the known path length.

@moklick
Copy link
Member

moklick commented Dec 4, 2024

Thanks for your help @mhuggins! I like the idea to pass svg attrs to the BaseEdge component but I am not sure if I would do the same for all built in edges. As soon as you want to do something fancy, you should use the BaseEdge and build your own edge component I would say. This would also make this change smaller since we would only need to update the BaseEdge component and not care about the prop drilling of the built in edges. wdyt?

@peterkogo
Copy link
Member

I removed quite a lot of manual prop drilling from some of the components. Some, like the straight edge, were using ...props already.

In reverse this means we have to prop drill for all components manually. Either way is fine for me.

@moklick
Copy link
Member

moklick commented Dec 4, 2024

The prop drilling was introduced with this PR I think. Current main: https://github.com/xyflow/xyflow/blob/main/packages/react/src/components/Edges/StraightEdge.tsx

@peterkogo
Copy link
Member

You're right! I must have mistaken the StepEdge for a regular Edge. Will revert it.

@peterkogo
Copy link
Member

@moklick done, it's better this way.

@moklick moklick merged commit ce8c2cf into xyflow:main Dec 4, 2024
1 check passed
@moklick moklick mentioned this pull request Dec 4, 2024
@mhuggins
Copy link
Contributor Author

mhuggins commented Dec 4, 2024

Thanks for helping see this through, everyone! 😄

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.

3 participants