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

[Abandoned, author unable to close or comment] Clean up layout issues, handle large text sizes. #319

Closed

Conversation

@gspencergoog
Copy link

gspencergoog commented Mar 26, 2020

What does this PR accomplish?

This cleans up some of the existing overflow issues, and helps support large font sizes that users can set on iOS (and Android too, but there they don't get nearly as large).

I also made the main menu and cards scrollable if they overflow, with a gradient hint at the bottom if there is more to see.

Fixes #303

Did you add any dependencies?

  • I bumped the min Dart version number to 2.3.0 from 2.1.0 to pick up many good Dart enhancements (list embedded conditionals, for one).

How did you test the change?

I tested it in Android and iOS simulators.

Home screen:
Regular user text size:
Screen Shot 2020-03-25 at 4 53 46 PM

iOS largest accessibility text size:
Screen Shot 2020-03-25 at 4 54 52 PM

Myth-busters:
Regular size text:
Screen Shot 2020-03-25 at 5 06 11 PM

iOS largest accessibility text size:
Screen Shot 2020-03-25 at 5 06 51 PM

Copy link
Collaborator

hspinks left a comment

again, I'm not experienced enough with Flutter to leave a meaningful review, but looks good. will wait on a review from a more experienced Flutter-er to approve then can give my 👍

child: Container(
padding: EdgeInsets.only(bottom: 20),
child: pageViewIndicator(context)),
child: Container(padding: EdgeInsets.only(bottom: 20), child: pageViewIndicator(context)),

This comment has been minimized.

Copy link
@hspinks

hspinks Mar 26, 2020

Collaborator

is there a preferred code style on this from flutter format? prefer to standardize once and stick with it then have changes in a PR to review that aren't related to the overall purpose of the PR. not a blocker on this PR, just a general comment...

This comment has been minimized.

Copy link
@sstur

sstur Mar 26, 2020

Collaborator

One style note on this, both usage of EdgeInsets above use const and use 10.0 but this uses the integer notation and no const. I don't know which one is preferable but we should probably be consistent.
Edit: I realize this is not your code, it just got reformatted.

This comment has been minimized.

Copy link
@gspencergoog

gspencergoog Mar 26, 2020

Author

I looked for a CONTRIBUTING.md and in the Wiki to see, but guidance doesn't appear to exist, so I'm not sure what the coding conventions should be. My suggestion is to define it as the output of flutter format -l 100 . and leave it at that. Easily enforceable, eliminates all the time and effort talking about and fixing formatting, and it does a pretty good job, even if it's not perfect.

This comment has been minimized.

Copy link
@json469

json469 Mar 27, 2020

If you would like to easily enforce but eliminate all the time and effort talking about formatting once-in-for-all 😆I would strongly recommend sticking to the style guide for Flutter's own repo. For example: preferred maximum line length is 80.

Would be greatly beneficial to all new contributors even if we just link the above official Flutter repo's style guide in CONTRIBUTING.md so we are all in the same page. I spent some time looking for one before I started to code but couldn't find anything.

@sstur
sstur approved these changes Mar 26, 2020
Copy link
Collaborator

sstur left a comment

This looks good to me.

@patniemeyer patniemeyer self-requested a review Mar 26, 2020
@patniemeyer

This comment has been minimized.

Copy link
Collaborator

patniemeyer commented Mar 26, 2020

I think we should consider advanced accessibility options to be a separate topic from general adaptive text within the standard range and standard variation in screen sizes. In general I think accommodating the full range of accessibility options may require some special work for special cases and I'm not sure that we'll be able to cover these in the first few releases. That said, thank you for looking at this and I'll review what you did ASAP.

@patniemeyer

This comment has been minimized.

Copy link
Collaborator

patniemeyer commented Mar 26, 2020

I think this looks good. A couple of things: 1) Wondering if we should center or offset the text when there is no image in the carousel? This is more of a design decision so not a blocker. 2) Was the explicit scroll controller necessary for the ScrollDownHint (I haven't used that before). Wondering if we could use PrimaryScrollController.of(context) here instead of creating one. I think that may be necessary to get iOS to do the scroll to top on double tap gesture.

Copy link
Collaborator

patniemeyer left a comment

Added some comments but I don't think they are blockers. Thanks!

@gspencergoog

This comment has been minimized.

Copy link
Author

gspencergoog commented Mar 26, 2020

The scroll controller is necessary, as the ScrollDownHint (a widget I added, not a Flutter widget) draws its hint on top of the scrollable, and so must be an ancestor of the Scrollable, which means that PrimaryScrollController.of won't be able to find the scrollable's controller. It uses the shared scroll controller to know when to display the hint.

I start-aligned the text when there is no icon. It looks a little better (in my non-designer engineer opinion), so I kept it that way, but of course a designer may disagree. It's easy to change.

Copy link
Collaborator

patniemeyer left a comment

Thanks for the work!

@patniemeyer

This comment has been minimized.

Copy link
Collaborator

patniemeyer commented Mar 26, 2020

@SamMousa I think this is ready to go but we need an approver with write permissions.

@SamMousa

This comment has been minimized.

Copy link
Member

SamMousa commented Mar 26, 2020

Will merge when at pc

@gspencergoog gspencergoog force-pushed the gspencergoog:clean_up branch from 791d77b to 331c3c7 Mar 26, 2020
@@ -30,13 +30,14 @@ class _MyAppState extends State<MyApp> {
S.delegate
],
supportedLocales: S.delegate.supportedLocales,
theme: ThemeData(
theme: ThemeData.light().copyWith(

This comment has been minimized.

Copy link
@theswerd

theswerd Mar 26, 2020

Collaborator

I believe theme defaults to light and this is unnecessary

This comment has been minimized.

Copy link
@gspencergoog

gspencergoog Mar 26, 2020

Author

It does, just being explicit. I'll revert this line.

Copy link
Collaborator

theswerd left a comment

Looks good!

@gspencergoog gspencergoog force-pushed the gspencergoog:clean_up branch from 2a3a17a to 17be379 Mar 27, 2020
@gspencergoog gspencergoog changed the title Clean up layout issues, handle large text sizes. [Abandoned, author unable to close or comment] Clean up layout issues, handle large text sizes. Mar 27, 2020
@advayDev1

This comment has been minimized.

Copy link
Collaborator

advayDev1 commented Mar 29, 2020

Now that the repo is opened up again, shall we investigate this in terms of the most recent design?

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

Successfully merging this pull request may close these issues.

8 participants
You can’t perform that action at this time.