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(*): GridRadial, GridAngle & GridPolar #1007

Merged
merged 4 commits into from
Feb 10, 2021

Conversation

sarathps93
Copy link
Contributor

@sarathps93 sarathps93 commented Jan 8, 2021

🚀 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.

image

@sarathps93
Copy link
Contributor Author

sarathps93 commented Jan 8, 2021

@williaster As promised earlier, this contains the reimplementation of your old PR #405.

@coveralls
Copy link

coveralls commented Jan 8, 2021

Pull Request Test Coverage Report for Build 556018669

  • 10 of 10 (100.0%) changed or added relevant lines in 4 files are covered.
  • 9 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.2%) to 61.977%

Files with Coverage Reduction New Missed Lines %
packages/visx-xychart/src/components/Tooltip.tsx 3 84.72%
packages/visx-tooltip/src/hooks/useTooltipInPortal.tsx 6 0%
Totals Coverage Status
Change from base Build 516508911: 0.2%
Covered Lines: 1801
Relevant Lines: 2768

💛 - Coveralls

@williaster
Copy link
Collaborator

Thanks for the PR @sarathps93 🙌 !!! Sorry for the delay in review, I should be able to get to it tomorrow.

Copy link
Collaborator

@williaster williaster left a 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';
Copy link
Collaborator

@williaster williaster Jan 14, 2021

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?

Copy link
Contributor Author

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-grid/src/grids/GridAngle.tsx Outdated Show resolved Hide resolved
packages/visx-grid/src/grids/GridAngle.tsx Outdated Show resolved Hide resolved
packages/visx-grid/src/grids/GridAngle.tsx Outdated Show resolved Hide resolved
packages/visx-grid/src/grids/GridPolar.tsx Outdated Show resolved Hide resolved
packages/visx-grid/src/grids/GridRadial.tsx Outdated Show resolved Hide resolved
packages/visx-grid/src/grids/GridRadial.tsx Outdated Show resolved Hide resolved
packages/visx-grid/src/grids/GridRadial.tsx Outdated Show resolved Hide resolved
packages/visx-grid/src/grids/GridRadial.tsx Outdated Show resolved Hide resolved
@sarathps93
Copy link
Contributor Author

@williaster sorry about the delay in addressing the review comments. I will push the changes over this weekend

@williaster
Copy link
Collaborator

@sarathps93 thanks for the heads up, no rush!

@sarathps93
Copy link
Contributor Author

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 Happo CI check pass.

@williaster
Copy link
Collaborator

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!

Copy link
Collaborator

@kristw kristw left a 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.

Copy link
Collaborator

@williaster williaster left a 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!

packages/visx-grid/src/grids/GridAngle.tsx Outdated Show resolved Hide resolved
packages/visx-grid/src/grids/GridAngle.tsx Outdated Show resolved Hide resolved
packages/visx-grid/src/grids/GridAngle.tsx Outdated Show resolved Hide resolved
packages/visx-grid/src/grids/GridRadial.tsx Outdated Show resolved Hide resolved
packages/visx-grid/src/grids/GridPolar.tsx Outdated Show resolved Hide resolved
packages/visx-grid/src/grids/GridPolar.tsx Outdated Show resolved Hide resolved
packages/visx-grid/src/grids/GridPolar.tsx Outdated Show resolved Hide resolved
packages/visx-grid/src/grids/GridPolar.tsx Outdated Show resolved Hide resolved
packages/visx-grid/src/grids/GridPolar.tsx Outdated Show resolved Hide resolved
packages/visx-grid/src/grids/GridPolar.tsx Outdated Show resolved Hide resolved
@williaster
Copy link
Collaborator

Sorry ... now debugging the build issue.

@sarathps93
Copy link
Contributor Author

Thanks @williaster . I am glad that I am able to contribute.

@trotzig
Copy link

trotzig commented Feb 10, 2021

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.

@trotzig
Copy link

trotzig commented Feb 10, 2021

I've found the issue. I'm working on a fix, should be deployed within an hour!

@trotzig
Copy link

trotzig commented Feb 10, 2021

Alright, new release of happo.io is live. I think the build can be retriggered and things should just work ™.

@williaster
Copy link
Collaborator

Woot! let's merge this puppy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants