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

Pie chart plot type added to TablePlotter #330

Merged
merged 9 commits into from
Oct 22, 2020
Merged

Conversation

robpoll
Copy link
Contributor

@robpoll robpoll commented Oct 19, 2020

Insert a description of your pull request (PR) here, and check off the boxes below when they are done.


Contributor checklist

  • 🎉 This PR closes Add pie chart option to TablePlotter #297.
  • 📜 I have broken down my PR into the following tasks:
    • Add pie chart option to TablePlotter
  • 🤖 I have added tests, or extended existing tests, to cover any new features or bugs fixed in this PR.
  • 📖 I have considered adding a new entry in CHANGELOG.md, and added it if should be communicated there.

@asnyv
Copy link
Collaborator

asnyv commented Oct 19, 2020

Congrats with making your first PR @robpoll 🎉

One comment: Think at least we should add the possibility to use a column as labels/names, as that would make it a lot easier to analyze results, see e.g.: https://plotly.com/python/pie-charts/

@robpoll
Copy link
Contributor Author

robpoll commented Oct 19, 2020

Thanks man! You've helped me a lot! @asnyv
I want to add more details to it. I just wanted to have my code reviewed :D
That's exactly what I was trying to add. The logic I wanted to use is to add "names" to the chart based on the selected "values", but unfortunately, I didn't find yet the code for it.

@asnyv
Copy link
Collaborator

asnyv commented Oct 19, 2020

Might be that you have to call it "labels" as it is defined here @robpoll https://plotly.com/python/reference/pie/

Copy link
Collaborator

@asnyv asnyv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍 One suggestion + you should add an entry in the changelog
(you can add a new point for PR #330 under UNRELEASED -> Changed)

Co-authored-by: Asgeir Nyvoll <47146384+asnyv@users.noreply.github.com>
@asnyv
Copy link
Collaborator

asnyv commented Oct 21, 2020

Could you add to the CHANGELOG.md as well @robpoll ?

@robpoll
Copy link
Contributor Author

robpoll commented Oct 21, 2020

Yes, is it now alright @asnyv ?

@asnyv
Copy link
Collaborator

asnyv commented Oct 21, 2020

Looks good to me at least 👍 any final comments @anders-kiaer or @HansKallekleiv ?

Copy link
Collaborator

@HansKallekleiv HansKallekleiv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm 👍

@anders-kiaer anders-kiaer changed the title pie chart plot type added to tableplotter Pie chart plot type added to TablePlotter Oct 22, 2020
@asnyv asnyv merged commit 8428a68 into equinor:master Oct 22, 2020
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.

Add pie chart option to TablePlotter
4 participants