-
Notifications
You must be signed in to change notification settings - Fork 719
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(*): GridRadial, GridAngle & GridPolar #1007
Conversation
@williaster As promised earlier, this contains the reimplementation of your old PR #405. |
Pull Request Test Coverage Report for Build 556018669
💛 - Coveralls |
Thanks for the PR @sarathps93 🙌 !!! Sorry for the delay in review, I should be able to get to it tomorrow. |
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.
@sarathps93 this is AMAZING 🙌 Thanks so much for porting over the older PR and adding types + tests 🤩 .
I had a few minor comments about type annotations and making a few small type changes. I tried to be as specific as possible with suggestions
so hopefully it's not too much work. Sorry again about the delayed review 🙏
import { animated, useSpring } from 'react-spring'; | ||
|
||
const green = '#e5fd3d'; | ||
export const blue = '#aeeef8'; | ||
const darkgreen = '#dff84d'; | ||
export const background = '#744cca'; | ||
const darkbackground = '#603FA8'; | ||
const bg = '#744cca'; |
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.
is this line needed since it matches background
and I think is unused?
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.
We are using this color as stroke
for AxisLeft
. I will just rename it accordingly
packages/visx-demo/src/sandboxes/visx-shape-line-radial/Example.tsx
Outdated
Show resolved
Hide resolved
@williaster sorry about the delay in addressing the review comments. I will push the changes over this weekend |
@sarathps93 thanks for the heads up, no rush! |
9fea2b1
to
0b513f6
Compare
Sorry for the long delay in working on the changes. I have addressed all the review comments @williaster @kristw . Though I am not sure the action needed from my side to make |
Hey @sarathps93 thanks for iterating on this, no worries on the happo diffs, there were a couple of sporadic changes that should stabilize in the next day or so. I'll try to get this reviewed tomorrow! |
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.
My comments are all addressed. Will leave it to @williaster for final stamp.
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.
@sarathps93 this looks great to me, sorry for the delay in review. I had a couple minor prop annotation suggestions.
I can go ahead and commit them and merge, tho, so you don't have to do anything else since you've already done so much. Thanks again for the addition!
Sorry ... now debugging the build issue. |
Thanks @williaster . I am glad that I am able to contribute. |
Hi folks, Henric from Happo here 👋 I'm currently debugging the happo build issue. I hope to have a clearer picture of what's going on soon -- I'll keep you posted. |
I've found the issue. I'm working on a fix, should be deployed within an hour! |
Alright, new release of happo.io is live. I think the build can be retriggered and things should just work ™. |
Woot! let's merge this puppy. |
🚀 Enhancements
This is PR is a part of #832 and it re-implements an old PR #405 in the latest version. It adds
<GridRadial />
,<GridAngle />
and<GridPolar />
components to @visx/grid, and updates the RadialLine demo to be implemented using them.