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.

Sign up for GitHub

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

Jump to bottom

[Pie Chart] New feature: added a badge widget in addition to title #443

Merged
merged 12 commits into from
Oct 6, 2020
Merged

[Pie Chart] New feature: added a badge widget in addition to title #443

merged 12 commits into from
Oct 6, 2020

Conversation

gmaggio
Copy link

@gmaggio gmaggio commented Sep 8, 2020 •
edited
Loading

I needed a feature to add some kind of creative freedom in creating the title. So I added a way to add a widget to represent the title of the section. However, you can still have the default title as an option in addition to this widget. Having it as a widget also makes it intuitive to create and easy for novice programmers.

pie_chart_sample_3

Main Changes:

  • PieChart:
    • added child widget to CustomPaint to render the badge widgets
    • made default duration static to reuse externally for consistency
  • PieChartSectionData:
    • title: changed default to value
    • added params for badge widget
  • PieChartPainter:
    • added function to provide position offsets to parent
    • renamed _drawTexts to _calculateOverlays to better represent the various functions
  • Misc:
    • Added typedef for position offsets map: GetPositionOffsetsFunction
    • Added test for PieChart's badge widget
    • Added sample source code for pie chart with badge widgets
    • Updated documentation

@imaNNeo imaNNeo changed the base branch from master to dev September 25, 2020 22:00
@imaNNeo
Copy link
Owner

imaNNeo commented Sep 25, 2020

Hi dude, I am sorry for the delay.
I have just changed this PR's target branch to dev.
Today we have made some changes on dev, Now there is a conflict.
Can you please fix it?
Thanks, I will review it ASAP.

@gmaggio
Pie-Chart: adding title widget [in progress]
592898b 
- Prepared sample file
- Added typedef for title offsets map - GetTitleOffsetsFunction
- PieChartPainter: added function to provide title offsets to parent
- PieChartSectionData: added titleWidget param
- PieChartSectionData: added conditions to title params & changed defaults to value
- PieChart: added child widget to CustomPaint to render the title widgets
@gmaggio
PieChartPainter: check if title widgets exist
d0e685d 
- PieChart: made default duration static
- Added animation to title wigets in sample chart
@gmaggio
PieChartData: changed all title widget to badge widget
52ae8d3 
- removed overwriting title if badge widget is set & allowed to display both
- added parameter for badge widget offset posiiton: badgePositionPercentageOffset

- Added doc comments to badge widget's variables & functions
- PieChart: skipped badge widgets creation if null in section
@gmaggio
PieChart: removed unnecessary Container for CustomMultiChildLayout
2cba107
@gmaggio
PieChartSectionData: updated doc comment for badgeWidget
ef87088
@gmaggio
Updated sample file with badgeWidgets set for each sections
9b16aaa
@gmaggio
PieChartPainter: renamed _drawTexts to better represent the various f…
5e29a6c 
…unctions

Insert sample in PieChartPage
@gmaggio
PieChartSectionData: added doc description about badge widget
8e8a663
@gmaggio
Added test for PieChart's badge widget
b3cdb28
@gmaggio
PieChartSample3: added credit for the svg icons used
db5a5b5
@gmaggio
Updated documentation to include badge widget
37c0eb3
@gmaggio
PieChart: Remove unnecessary brackets from List.generate
f7dde1d
@gmaggio gmaggio force-pushed the feature/pie-chart-title-widget branch from 7b7a321 to f7dde1d Compare September 26, 2020 17:38
@gmaggio
Copy link
Author

gmaggio commented Sep 26, 2020

Hey, thanks for the reponse 😄
I have fixed the conflict. Looking forward to your review. 🙏 😇

@ulusoyca
Copy link

ulusoyca commented Oct 2, 2020

@gmaggio Thanks for the PR. It is a huge contribution to my project also. I am waiting for the merge too. Looks like the conflicts are resolved. @imaNNeoFighT no pressure but you would make me happy too if you have time to test and possibly merge this :)

@imaNNeo
Copy link
Owner

imaNNeo commented Oct 2, 2020

I'm on it :)

@imaNNeo
Copy link
Owner

imaNNeo commented Oct 2, 2020

Hey @gmaggio.
First of all, I want to let you know that it is the most beautiful and complete pull request I have ever had.
I appreciate your great solution and great work, I bet you it helps people a lot. And they are going to be happy.
You did a great job!

There is only one minor suggestion from me. Now we don't need the title property in PieChartSectionData anymore and we can remove it. Because we can have a Text widget instead.

We can define a Text widget as a default value for the badge widget and show the index of each section inside it.
How do you think?

@imaNNeo
Copy link
Owner

imaNNeo commented Oct 2, 2020

Oh, I see you have used both of them at the same time.
Now you can skip my suggestion, but I would like to hear your idea.
Thanks!

@imaNNeo
Copy link
Owner

imaNNeo commented Oct 2, 2020 •
edited
Loading

I think we can keep title to prevent making a breaking-change.
If you think we don't need it anymore, we can mark it deprecated!
I need your idea.
Thanks!

@gmaggio
Copy link
Author

gmaggio commented Oct 4, 2020

Hi @imaNNeoFighT. Thank you for the complement. I really appreciate you looking into my contribution. 😇
In regards to the title, I think we should just keep it for a couple of reasons:

  • It uses CustomPainter, which paints directly on the canvas during the painting phase.
    Though, to be honest, I'm not well versed on canvas painting, so my current assumption is that it is better than widget manipulation it terms of performance. Correct me if I'm wrong. 🤔
  • The badge widget has yet to be tested in various use cases and scenarios.
    I had only tried it for my personal needs and it is working okay so far. But we need to allow time for others to use it as well for better feedback. Let's hope it is working as intended, though. 😁
  • Keeping the title gives users more options.
    The title is technically more intuitive since it's more straight forward to use. Most people should be sufficient with this. The badge widget, on the other hand, allows customisation to the regular title but with an added effort, especially if animation is to be used. So we can leave the decision to the users to use whichever is suitable for their needs. And, of course, you can even use both of them at the same time, as you already know. 😉

These are only my point of views, though. Let me know what you think.
Cheers!

@imaNNeo
Copy link
Owner

imaNNeo commented Oct 6, 2020

All of your points make sense, and we will have both.
Thank you again!

@imaNNeo imaNNeo merged commit aed79a4 into imaNNeo:dev Oct 6, 2020
@imaNNeo imaNNeo mentioned this pull request Oct 6, 2020
Merged
@imaNNeo imaNNeo mentioned this pull request Mar 11, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gmaggio @imaNNeo @ulusoyca