Chart height on tablets #576
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
24bartixx
requested review from
simon-the-shark,
mikolaj-jalocha and
tomasz-trela
|
so afaik, the idea was to just limit the width e.g. to length (or 2* the length) of the shorter side of the device. Cause when we have full width, even full height is not enough for a readable chart. |
24bartixx
force-pushed
the
bug/chart-horizontal-view
branch
from
3bdca97 to
ed57791
Compare
There was a problem hiding this comment.
not exatcly what I've meant - but at least for the tablet it looks better. So we can go with this. Please test it on smaller phone as well, cause I suspect 1/2 of the height will look bad there. So adjusting this value for different screen sizes might be needed. Unless I'm wrong
| } | ||
| }).toList(), | ||
| // child: AspectRatio( | ||
| // aspectRatio: 1.5, |
There was a problem hiding this comment.
dead code :P - not nice
| maxNumberOfUsers: maxNumberOfUsers, | ||
| chartData: chartData, | ||
| SizedBox( | ||
| height: 0.5 * height, |
There was a problem hiding this comment.
it does look good on tablets, but please test it on smaller phone in horizontal view as well
|
@ellipsis review this |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 5c73240 in 1 minute and 30 seconds
More details
- Looked at
218lines of code in3files - Skipped
0files when reviewing. - Skipped posting
8drafted comments based on config settings.
1. lib/features/sks/sks_chart/presentation/sks_chart_card.dart:47
- Draft comment:
The height is hardcoded (0.5 * height). Add a comment clarifying why this factor is used and consider making it configurable. - 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 0.5 multiplier is indeed hardcoded and its purpose isn't immediately clear. However, this appears to be a UI layout decision - it's making the chart take up half the available height. UI layout constants like this are common and often don't need explanation. Making it configurable would add complexity without clear benefit.
The multiplier could be significant for the chart's visual design and making it configurable could allow for better adaptability in different contexts.
While true, UI layout constants are typically left as implementation details unless there's a specific need for configurability. No evidence suggests this needs to be configurable.
Delete the comment. This appears to be a straightforward UI layout decision, and the comment doesn't identify a clear issue that needs fixing.
2. lib/features/sks/sks_chart/presentation/sks_chart_sheet.dart:53
- Draft comment:
Changed ListView physics from NeverScrollableScrollPhysics to BouncingScrollPhysics. Confirm this change is intentional for the UX on tablets. - 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%
This is a UI/UX change related to scrolling behavior. The comment is asking for confirmation of intention, which violates our rules. It's not pointing out a clear issue, just asking if the change was intentional. We should assume the author made this UI change deliberately.
Maybe this scroll physics change could cause actual functional issues on tablets that I'm not considering? Maybe there's a platform-specific bug being introduced?
Without clear evidence of an actual bug or issue, we should trust that the UI change was intentional. Scroll physics are a UI concern that the author likely tested.
Delete this comment as it merely asks for confirmation of a UI change without identifying any concrete issues.
3. lib/features/sks/sks_chart/presentation/chart_elements/sks_chart.dart:27
- Draft comment:
Removal of the AspectRatio wrapper: ensure that the chart scales well on all devices. If a fixed aspect ratio is important, consider using a responsive container or extracting the value into configuration. - Reason this comment was not posted:
Comment looked like it was already resolved.
4. lib/features/sks/sks_chart/presentation/chart_elements/sks_chart.dart:108
- Draft comment:
Remove commented-out AspectRatio code if it's no longer needed to keep the file clean. - Reason this comment was not posted:
Marked as duplicate.
5. lib/features/sks/sks_chart/presentation/sks_chart_card.dart:48
- Draft comment:
Using a magic multiplier (0.5 * height) to set the chart's height may reduce clarity. Consider extracting this value into a configuration constant or adding a comment to explain its rationale. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
This appears to be a UI layout decision where the chart is intentionally sized to half the available height. The multiplier 0.5 is a simple, common ratio that's self-explanatory in UI layouts. The height parameter was just added, suggesting this is an intentional design change to make the chart height configurable. Magic numbers are generally bad, but 0.5 in UI layouts is a common, clear ratio.
The comment raises a valid point about magic numbers in code. Constants can improve maintainability.
However, 0.5 is a simple, clear ratio commonly used in UI layouts - it's not truly a "magic number" that needs explanation. The code is more readable with the direct calculation.
The comment should be deleted as it suggests unnecessary complexity for a simple, clear UI layout calculation.
6. lib/features/sks/sks_chart/presentation/sks_chart_sheet.dart:53
- Draft comment:
Changing the ListView physics to BouncingScrollPhysics may affect user experience on non-iOS platforms. Confirm that this behavior is intended across all target devices. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to confirm their intention regarding a change in ListView physics. This violates the rule against asking for confirmation of intention. The comment does not provide a specific code suggestion or ask for a test to be written, which would make it acceptable.
7. lib/features/sks/sks_chart/presentation/chart_elements/sks_chart.dart:44
- Draft comment:
Typo detected: 'orangePomegranade' appears to be a misspelling. It likely should be 'orangePomegranate'. Please correct the spelling. - Reason this comment was not posted:
Comment was on unchanged code.
8. lib/features/sks/sks_chart/presentation/sks_chart_sheet.dart:34
- Draft comment:
Typographical error: the parameter 'prefferedHeightFactor' is misspelled. Consider renaming it to 'preferredHeightFactor' for clarity. - 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_lQ2A1hSEDp0j2vtZ
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| ); | ||
| } | ||
| }).toList(), | ||
| // child: AspectRatio( |
There was a problem hiding this comment.
Remove commented-out AspectRatio widget if it's no longer needed to keep the code clean.
|
What's the status for this? If it's ready please launch some photos showing current situation. Try to resolve conflicts as well |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Let me know if it can be kind of hardocoded like this. I tried fixing it better a long time and I failed. If I have to do it in an elegant way, it's gonna be painful.... @simon-the-shark @mikolaj-jalocha @tomasz-trela
Before
After
l10n.yaml examplanation - without synthetic-package: false - doesn't generate l10n files without it
Important
Adjust chart height for tablets by removing
AspectRatioand adding dynamic height control inSksChartCard.AspectRatiofromSksChartinsks_chart.dartto allow dynamic height adjustment.heightparameter toSksChartCardinsks_chart_card.dartto control chart height.SksChartSheetinsks_chart_sheet.dartto passsheetHeighttoSksChartCard.physicsinSksChartSheetfromNeverScrollableScrollPhysicstoBouncingScrollPhysics.This description was created by
for 5c73240. It will automatically update as commits are pushed.