Fix/issue 895 #1118
Fix/issue 895 #1118
Conversation
|
Ack, I forgot to run the formatter before I pushed. I'll push again later. |
package.json
Outdated
| "sideEffects": false | ||
| "sideEffects": false, | ||
| "dependencies": { | ||
| "circle2": "^1.1.1" |
There was a problem hiding this comment.
I would really prefer not to take this dependency. Can you get it done without?
There was a problem hiding this comment.
I can give it a try! Do you have some general guidelines on whether or not to take a dependency, like a best practices guide? I didn't see anything about that in the formidable-playbook.
There was a problem hiding this comment.
Assuming we did want this dep, isn't it supposed to be in https://github.com/FormidableLabs/victory/blob/master/packages/victory-bar/package.json ?
There was a problem hiding this comment.
Yes, keeping it in the main package.json would add it to every package.
There was a problem hiding this comment.
Thanks for pointing that out! I'll uninstall it from the main package.json.
packages/victory-bar/src/bar.js
Outdated
| const topRightArc = `A ${topArc}, ${intrxnRight.x}, ${intrxnRight.y}`; | ||
| const bottomRightArc = `A ${bottomArc}, ${x1 - cornerRadius.bottom}, ${y0}`; | ||
| const end = "z"; | ||
| // There are four ways the verticalBarPath can self-intersect |
|
Do Not Merge! Just pushing to get feedback. I removed the circle2 dependency with my own circle module and wrote tests for that module. I am considering some changes to make
I ready though the testing section of |
|
Makes sense to me 👍Thanks for the feedback. |
| this.x = x; | ||
| this.y = y; | ||
| } | ||
| const Point = function (x, y) { |
There was a problem hiding this comment.
Writing function Point(x, y) results in ESLint error func-style
Writing const Point = function (x, y) results in ESLint error no-invalid-this
Which is preferred?
There was a problem hiding this comment.
const Point = (x, y) => ... should pass our linting rules. Otherwise, prefer function Point(x, y) and add an exception for func-style
|
@boygirl Would I be a good idea to solve issue #1119 in this PR/branch as well? It would be kinda strange to solve issue #895, and then immediately edit the same code to solve #1119. Let me know what you think of changes, and if it looks good to you, I can apply the fix to horizontal bars and solve #1119. |
|
@narinluangrath yes, I agree those changes make sense together. |
|
@narinluangrath to clarify, yes, please do add #1119 into this ticket. I also think it makes sense to move these path calculations into a separate file since they're getting so lengthy. Maybe |
- Translate code from getVerticalBarPath to horizontal case - Add Circle.ProtoType.solveY helper method
- Allow topLeft, topRight, bottomLeft, bottomRight fields - Still need to make it functional
- modify getCornerRadius helper function to look for topLeft, topRight (not just top) - create helper function getVerticalBarPoints to make getVerticalBarPath function smaller - add temporary solution for getHorizontalBarPath and getPolarBarPath (see TODO)
|
Going to push fix after work today! Code is done, just need to pass linting rules. |
- Add temporary solution for getVerticalPolarBar since it normally doesn't accept topLeft, topRight, etc. - Change cornerRadius function to satisfy linter - Try to change getVerticalBarPoints to satisfy linter
|
Gurrr merge conflict. I probably should have seen this coming, since most of the conflict is caused by my own code from a previously merged PR ... going to rebase and try again. |
|
@narinluangrath closing this one in favor of #1158 |
What's new:
Other comments: