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

Task/#269 ui fixes graphs #274

Merged
merged 16 commits into from
Jul 6, 2020
Merged

Task/#269 ui fixes graphs #274

merged 16 commits into from
Jul 6, 2020

Conversation

OldMetalmind
Copy link
Member

Description

Not everything is possible to be fixed given the limitations of the library used.

Labels

βœ… - Fixed
🚫 - Not possible to fixe given current limitations
❓ - Possible, will be fixed in a new PR

Changes

High Priority

  • βœ… Axis styling should use Roboto Condensed.
  • βœ… Axis should have an easier to understand interval. For example, each 5000 instead of each 3100.
  • βœ… X Axis should not have tilted text. Instead, reduce the interval in which text appears, like once per month.
  • ❓ Vertical bars should shrink in the "Tudo" view, keeping 1dp of distance.
  • ❓ On selection, the selected bar should be dark green
  • 🚫 The selection bubble should have the selected day
  • ❓ The selection bubble should not have decimals
  • βœ… The data on the "Proporção UCI" doesn't seem correct.
  • 🚫 Some graphs were designed to be horizontal. I understand it may be difficult to make entirely new graphs for this, but the readability is impacted if they're vertical.

Medium priority

  • ❓ Distances appear to be wrong. Please confirm with the Zeplin project.
  • ❓ Missing graph for Geographic zones?
  • ❓ Vertical lines should not have the same style as the horizontal ones.

Small priority

-❓ Style details that don't have a major impact on usability

@OldMetalmind OldMetalmind requested review from danidroid, Vanethos and miguelpruivo and removed request for danidroid and Vanethos July 5, 2020 19:13
Copy link
Member

@miguelpruivo miguelpruivo left a comment

Choose a reason for hiding this comment

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

Overall looks good to me, I've suggested some optional changes. Apply it if you have the time, otherwise it's ok to merge.

),
),
checkHasAgeGroupData(currentStatistics.confirmedRecentByAgeGroup)
? Container()
Copy link
Member

Choose a reason for hiding this comment

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

Since this is in a Column we can avoid ternary that will add an "invisible" widget to the tree. We can make it if( checkHasAgeGroupData(currentStatistics.confirmedRecentByAgeGroup)) Padding(...) instead.

male,
female,
}) {
this.male = male == null ? 0 : male;
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather have this 0 as default values on the constructor.

@@ -13,6 +13,31 @@

import 'dart:math' as math;

///Possible intervals
List<double> intervals = [
Copy link
Member

Choose a reason for hiding this comment

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

Make it const.

@OldMetalmind OldMetalmind requested a review from miguelpruivo July 6, 2020 11:38
@miguelpruivo miguelpruivo merged commit ed238d2 into dev Jul 6, 2020
@miguelpruivo miguelpruivo deleted the task/#269_UI_fixes_graphs branch July 6, 2020 12:01
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