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

Adding Piechart support #291

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

baszach
Copy link

@baszach baszach commented May 25, 2021

Greetings,
I added the Pie trace and some needed elements (e.g. Direction or PieTitleFont). Is the written code going in the right direction? Also, should I write a test in the DocumentationTests class?

baszach added 2 commits May 24, 2021 16:30
feat: added pie elements for PieChart support
@alexarchambault
Copy link
Owner

alexarchambault commented May 25, 2021

Thanks! You might want to add things in DocumentationTests, yes, and see how it goes, by adding basic/pie in this list. This will try to run these examples in the tests.

I also see you added 3 enumeration (Direction, PieTextPosition, PieTitlePosition). You need to state these are enumerations, by adding a line for each of them around here. That allows them to be encoded / decoded as strings.

@baszach
Copy link
Author

baszach commented May 26, 2021

Hello! I proceeded as you suggested and implemented a test, registered the enums etc etc...

When I run the test DocumentationTests it fails for the pie chart. The error says Decoding data: Found extra fields: textinfo, color. I think it's because I haven't implemented every possible field from here. Now my questions are: Why don't these tests fail for other Traces, as they don't implement each field either? And is the current state of the PR sufficient or is something missing?

Thank you in advance :)))))

@baszach
Copy link
Author

baszach commented Jun 16, 2021

Hello @alexarchambault
did you manage to take a look at my last commits? It would be great if you could give me feedback so I can make the changes, if necessary.
Thanks :)

@baszach
Copy link
Author

baszach commented Jul 25, 2021

Hello again,
can this PR be merged or is there anything missing? Would be cool to be able to use that pie chart :)
@alexarchambault thanks

@tflucke
Copy link

tflucke commented Jun 11, 2022

@alexarchambault I would also like to express interest in getting this merged in.

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

Successfully merging this pull request may close these issues.

3 participants