-
Notifications
You must be signed in to change notification settings - Fork 715
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(sankey): add @visx/sankey #1880
Conversation
@williaster When you have a moment to approve the workflow again, I believe I've resolved the steps (at least locally) so that it will complete. That said, I'm not sure what the happo output will be, given there are new additions to the demo site. |
@williaster Happy to help resolve any issues surfaced in happo, if you can point me to them? I suspect most of the issues are because of the newly added examples/docs? |
hey @jacksonhardaker happo looks good to me (sorry it's opaque to contributors, we can't open up access to that check unfortunately). the diff it flagged was just a new addition as you suspected thanks so much for the contribution, will try and get this reviewed with @hshoff in the next few days. thanks for your patience! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @jacksonhardaker thank you SO MUCH for this awesome contribution. everything looks spectacular, we appreciate how well you followed code styles/all of the integration points for a new package including tests and a gallery example with sandbox 🙏 🙏 🙏
major apologies on the slow review, it's been a really busy time for the maintainers. I had 2x super small comments and then I think it's good to go!! 🚀 THANK YOU AGAIN! it's always exciting to have new packages and functionality.
@@ -45,6 +46,8 @@ import { packageJson as textPackageJson } from '../components/Gallery/TextTile'; | |||
import extractVisxDepsFromPackageJson from '../components/util/extractVisxDepsFromPackageJson'; | |||
import { VisxPackage } from '../types'; | |||
|
|||
console.log({ sankeyPackageJson }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reminder to remove this
"@types/react": "*", | ||
"@visx/group": "3.3.0", | ||
"classnames": "^2.3.1", | ||
"d3-sankey": "^0.12.3", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
noting for my future self that this package doesn't have an esm-only
version yet, so doesn't need to be in @visx/vendor
. d3-sankey
itself has dependencies on d3-array
and d3-shape
(and transitive internmap
) but they aren't ESM-only versions
@@ -0,0 +1,19 @@ | |||
export { default as Sankey } from './Sankey'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks great, thanks for adding the relevant types (including SankeyProps
) as well 💯
</svg>, | ||
); | ||
|
||
describe('<Sankey />', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏 ty!! these are great
Thanks @williaster. I made those small tweaks, so should be good to go. |
amazing! lgtm, will merge this and it should be released/live soon! thanks again for the great work here 🙌 |
🎉 This PR is included in version |
🚀 Enhancements
d3-sankey