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

feat(parkings): add hour label and enable dots #647

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

Conversation

mmzek
Copy link
Member

@mmzek mmzek commented Mar 15, 2025

I added hour label and removed dots.
image


Important

Add hour labels to parking chart tooltips and disable dots on chart lines.

  • Behavior:
    • Added hour labels to tooltips in chart_widget.dart.
    • Disabled dots on chart lines in chart_line.dart.
  • Configuration:
    • Removed showDots from ParkingChartConfig in ui_config.dart.
  • Dependencies:
    • Updated fl_chart version to ^0.70.2 in pubspec.yaml.

This description was created by Ellipsis for ee3a8f7. It will automatically update as commits are pushed.

@mmzek mmzek linked an issue Mar 15, 2025 that may be closed by this pull request
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot 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 to me! Reviewed everything up to a132389 in 1 minute and 42 seconds

More details
  • Looked at 143 lines of code in 7 files
  • Skipped 0 files when reviewing.
  • Skipped posting 16 drafted comments based on config settings.
1. lib/config/ui_config.dart:122
  • Draft comment:
    Removed showDots config and changed generateRangeHourPoints parameters. Confirm this removal is deliberate.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
2. lib/features/parkings/parking_chart/chart_elements/chart_line.dart:15
  • Draft comment:
    dotData is now disabled (show:false). Ensure this is in line with intended behavior; PR title says ‘enable dots’ but description mentions removing dots.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
    The comment is asking for verification and pointing out a discrepancy with PR metadata. Our rules explicitly state not to ask for verification or make comments about PR descriptions. The actual code change seems intentional - replacing a complex dot showing logic with a simple false value.
    Maybe the discrepancy between PR title and code indicates a real bug that needs to be fixed? The suggestion to enable dots could be correct.
    Per our rules, we should not speculate based on PR metadata or ask for verification. The code change itself appears intentional and clear - simplifying dot display logic.
    Delete this comment as it violates our rules by asking for verification and referring to PR metadata rather than making a clear, actionable code suggestion.
3. lib/features/parkings/parking_chart/chart_elements/labels_bottom.dart:18
  • Draft comment:
    Updated lambda to use TitleMeta parameter; good update for new fl_chart API.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
4. lib/features/parkings/parking_chart/chart_elements/labels_left.dart:22
  • Draft comment:
    Replaced 'axisSide' with 'meta' in SideTitleWidget; aligns with updated API.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
5. lib/features/parkings/parking_chart/utils/chart_utils.dart:18
  • Draft comment:
    Removed unused 'showDotOrNot' in favor of disabling dots. Code cleanup seems fine.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
6. lib/features/parkings/parking_chart/widgets/chart_widget.dart:49
  • Draft comment:
    Tooltip now concatenates hour label with value. Check if the newline formatting meets UI requirements.
  • Reason this comment was not posted:
    Confidence changes required: 30% <= threshold 50%
    None
7. pubspec.yaml:81
  • Draft comment:
    Updated fl_chart version to ^0.70.2. Verify compatibility with other dependencies.
  • Reason this comment was not posted:
    Confidence changes required: 30% <= threshold 50%
    None
8. lib/config/ui_config.dart:122
  • Draft comment:
    Verify intended removal of 'showDots' and change in 'showLabels' step (from step=2 to default).
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
9. lib/features/parkings/parking_chart/chart_elements/chart_line.dart:15
  • Draft comment:
    Dot display is now hard-coded as off ('show: false'); confirm if this is intended since PR title mentions enabling dots.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
    The comment violates several rules: 1) It asks for confirmation of intention 2) It references PR title which is external context 3) It makes a suggestion based on speculation about requirements rather than clear technical issues. The change from a utility function to a simple boolean is a valid refactoring choice.
    Maybe the PR title does indicate a real requirement to show dots that this change contradicts. Maybe the removed utility had important logic.
    Without seeing the PR title or utility code, we can't make assumptions. The code change itself is clean and valid. If showing dots was required, that should be raised with clear evidence, not speculation.
    Delete the comment as it violates rules against asking for confirmation and making speculative suggestions without clear evidence of an issue.
10. lib/features/parkings/parking_chart/chart_elements/labels_bottom.dart:18
  • Draft comment:
    Update to the new fl_chart API using TitleMeta in getTitlesWidget looks correct. Verify that label rotation and padding still render as expected.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
11. lib/features/parkings/parking_chart/chart_elements/labels_left.dart:22
  • Draft comment:
    Changed property in SideTitleWidget from 'axisSide' to 'meta'; ensure this usage aligns with the updated fl_chart API.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
12. lib/features/parkings/parking_chart/utils/chart_utils.dart:19
  • Draft comment:
    Removal of the 'showDotOrNot' helper appears intentional now that dots are disabled; confirm no other code depends on this logic.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
13. lib/features/parkings/parking_chart/widgets/chart_widget.dart:48
  • Draft comment:
    Tooltip now includes an hour label with a newline; check that the formatting meets UX expectations.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
14. pubspec.yaml:81
  • Draft comment:
    fl_chart version updated to ^0.70.2; ensure all API changes are handled in the code.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
15. lib/config/ui_config.dart:221
  • Draft comment:
    Typo: Consider renaming 'symetricalPaddingSmall', 'symetricalPaddingMedium', and 'symetricalPaddingBig' to use the correct spelling 'symmetrical' to improve code readability and consistency.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
16. lib/config/ui_config.dart:243
  • Draft comment:
    Typo in comment: The term 'accessability' should be corrected to 'accessibility' to ensure proper spelling in documentation.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_Y8SCCttUQQF6ib5c


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@mikolaj-jalocha
Copy link
Member

LGTM. Wdyt about adding some color to hour label? Just to distinguish it a little bit?

@pull-request-size pull-request-size bot added size/S and removed size/M labels Mar 15, 2025
@mmzek mmzek self-assigned this Mar 15, 2025
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot 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 to me! Incremental review on ee3a8f7 in 1 minute and 15 seconds

More details
  • Looked at 75 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 9 drafted comments based on config settings.
1. lib/config/ui_config.dart:122
  • Draft comment:
    Inconsistent commit message: title says 'enable dots' but description states 'removed dots'. Clarify which behavior should be used.
  • Reason this comment was not posted:
    Comment was on unchanged code.
2. lib/features/parkings/parking_chart/chart_elements/labels_bottom.dart:12
  • Draft comment:
    Code formatting changes appear purely stylistic. Confirm this adheres to your project's style guidelines.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
3. lib/features/parkings/parking_chart/utils/chart_utils.dart:6
  • Draft comment:
    Removal of the unused 'fl_chart' import improves cleanliness. Ensure it's not required elsewhere.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
4. lib/config/ui_config.dart:122
  • Draft comment:
    Minor formatting: extra blank line removed in ParkingChartConfig. Ensure consistent spacing across config classes.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
5. lib/features/parkings/parking_chart/chart_elements/labels_bottom.dart:11
  • Draft comment:
    The lambda in getTitlesWidget has been reformatted for clarity – looks good.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
6. lib/features/parkings/parking_chart/utils/chart_utils.dart:6
  • Draft comment:
    Unused import 'package:fl_chart/fl_chart.dart' was removed – good cleanup.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
7. lib/features/parkings/parking_chart/utils/chart_utils.dart:19
  • Draft comment:
    Ensure HourLabel overrides equality and hashCode so that membership checks using contains work as intended.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
8. lib/config/ui_config.dart:220
  • Draft comment:
    Typo: 'symetricalPaddingSmall', 'symetricalPaddingMedium', and 'symetricalPaddingBig' should be renamed to 'symmetricalPaddingSmall', 'symmetricalPaddingMedium', and 'symmetricalPaddingBig' for correct spelling.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. lib/config/ui_config.dart:244
  • Draft comment:
    Typo: In the comments for accessibilityLevelColors, 'unaccessible' should be corrected to 'inaccessible' for accuracy.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_YzGbY9ztr76eFa38


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

mikolaj-jalocha

This comment was marked as duplicate.

@simon-the-shark simon-the-shark changed the title feat(parking-chart): add hour label and enable dots feat(parkings: add hour label and enable dots Mar 15, 2025
@simon-the-shark simon-the-shark changed the title feat(parkings: add hour label and enable dots feat(parkings): add hour label and enable dots Mar 15, 2025
Copy link
Member

@simon-the-shark simon-the-shark left a comment

Choose a reason for hiding this comment

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

lgtm but I would make it

value\nhour and not hour\nvalue but that's my personal opiion

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

Successfully merging this pull request may close these issues.

feat(parking-chart): fix dots and add hour label
3 participants