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

Fix padding and margin for LineLabels and Tooltips #824 #1587

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

Conversation

k0psutin
Copy link
Contributor

I wanted to make a PR to check with you @imaNNeo that, this PR is going on the right direction. I still need to make more testing, so a draftish PR.

This PR is for #824 and there were also discussion about adding padding/margin for the tooltips as well.

Copy link

codecov bot commented Feb 17, 2024

Codecov Report

Attention: Patch coverage is 95.09804% with 5 lines in your changes missing coverage. Please review.

Project coverage is 87.53%. Comparing base (930ac12) to head (0e8b9ad).
Report is 21 commits behind head on main.

Files Patch % Lines
lib/src/chart/line_chart/line_chart_painter.dart 64.28% 5 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1587   +/-   ##
=======================================
  Coverage   87.52%   87.53%           
=======================================
  Files          45       45           
  Lines        3078     3072    -6     
=======================================
- Hits         2694     2689    -5     
+ Misses        384      383    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lib/src/chart/base/axis_chart/axis_chart_data.dart Outdated Show resolved Hide resolved
lib/src/chart/base/axis_chart/axis_chart_data.dart Outdated Show resolved Hide resolved
lib/src/chart/base/axis_chart/axis_chart_painter.dart Outdated Show resolved Hide resolved
lib/src/chart/bar_chart/bar_chart_data.dart Show resolved Hide resolved
lib/src/extensions/rect_extension.dart Outdated Show resolved Hide resolved
@k0psutin
Copy link
Contributor Author

k0psutin commented Mar 22, 2024

What about these changes? There are no more margin parameters, everything is done by vertical and horizontal offsets. I had to generate mockito files again, so I could write more tests, but I have no idea why it bumped the Mockito version on the comments.

// Mocks generated by Mockito 5.4.4 from annotations

@imaNNeo
Copy link
Owner

imaNNeo commented May 8, 2024

HI, can you please resolve the conflicts?
Then I can review it

@k0psutin k0psutin force-pushed the feature/padding-for-line-touch-tooltip-data branch from 437af1f to d1ade1b Compare May 12, 2024 17:01
@k0psutin
Copy link
Contributor Author

k0psutin commented May 12, 2024

There were many conflicts, but hopefully everything is okay now. :) Not sure about the changelog format though.

@k0psutin k0psutin force-pushed the feature/padding-for-line-touch-tooltip-data branch from 3ba03f4 to 7334849 Compare June 28, 2024 12:06
@k0psutin
Copy link
Contributor Author

Resolved conflicts again.

@imaNNeo
Copy link
Owner

imaNNeo commented Jul 19, 2024

Everything seems good to me.
So please resolve those two discussions, and then we can take a look again at the changelog and merge!

@k0psutin k0psutin force-pushed the feature/padding-for-line-touch-tooltip-data branch from 7334849 to 6303a49 Compare July 21, 2024 12:30
@imaNNeo
Copy link
Owner

imaNNeo commented Aug 26, 2024

Can you please rebase again?
Also, please don't update the CHANGELOG.md, because it is getting updated all the time and there will be conflicts.
Please write your proposed changelog here, I will add it to the file. Thanks!

@k0psutin k0psutin force-pushed the feature/padding-for-line-touch-tooltip-data branch from 0e8b9ad to 1eaf49c Compare August 27, 2024 18:25
@k0psutin
Copy link
Contributor Author

Changelog:


* **BREAKING**  (by @k0psutin) Remove `margin` from HorizontalLineLabel and VerticalLineLabel, #824
* **BREAKING** (by @k0psutin) Remove `bottomMargin` from ScatterTooltipItem , #824
* **BREAKING**  (by @k0psutin) Remove `tooltipMargin` from HorizontalLineLabel and VerticalLineLabel, #824
* **BREAKING** (by @kopsutin) Add property `tooltipVerticalOffset` to BarTouchTooltipData, LineTouchTooltipData and ScatterTooltipItem to replace `tooltipMargin`, #824 
* **IMPROVEMENT** (by @k0psutin) Add `tooltipPadding` to BarTouchTooltipData, LineTouchTooltipData and ScatterTouchTooltipData, #824
* **IMPROVEMENT** (by @k0psutin) Add `verticalOffset` and `horizontalOffset` to HorizontalLineLabel and VerticalLineLabel to replace `margin`, #824  
* **BUGFIX**  (by @k0psutin) Fix `padding` to apply padding correctly in HorizontalLineLabel and VerticalLineLabel, #824
/// Migration guide:
/// Old way:
BarTouchTooltipData(
  tooltipMargin: -10,
)

/// New way:
BarTouchTooltipData(
  tooltipVerticalOffset: -10, // same effect as old tooltipMargin
  tooltipPadding: const EdgeInsets.symmetric(horizontal: 16, vertical: 8), // Adds padding to tooltip
)
/// Migration guide:
/// Old way:
ScatterTooltipItem(
  bottomMargin: 10,
)

/// New way:
ScatterTouchTooltipData(
  tooltipVerticalOffset: 10, // same effect as old tooltipMargin
  tooltipPadding: const EdgeInsets.symmetric(horizontal: 16, vertical: 8), // Adds padding to tooltip
)

@k0psutin
Copy link
Contributor Author

@imaNNeo Is this feature still relevant? Should I continue updating/merging or should I focus on other features?

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.

2 participants