-
Notifications
You must be signed in to change notification settings - Fork 120
Draft: implement LayoutPolar
#355
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
Conversation
LayoutPolar (削除ここまで)LayoutPolar (追記ここまで)
Looks good so far.
If you could add an example in examples/, then I can expand the mdBook such that it is also included in the book.
plotly/src/layout/polar.rs
Outdated
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.
I prefer we keep them separate for now. I have done similar for other properties. Eventually, I might look into using a macro to generate these.
e8fa562 to
3d76518
Compare
3d76518 to
471e9f0
Compare
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.
@dustypomerleau don't run plot.show() in tests as this will open the plot in the system browser. For testte we usually check if the serialization to JSON is correct. If you were doing that to check your implementation, better build a simple example in examples ...
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.
Apologies, that's not really the test I plan to have in that location. I was just using it to check my work. Once I get everything working, I'll move what I've got into a (simpler) example and put a serialization test in the module. I can push to another branch so you're not seeing all this noise.
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.
Sure, no worries. Makes sense. Just wanted to point that out since in the CI calling plot.show() will hang
Uh oh!
There was an error while loading. Please reload this page.
Happy for any feedback/advice while I work on this. Eventually closes #352.