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

Change the color and visibility of the icons on the status bar for the Overview and MyPICOS #144

Conversation

FaysalSolutions
Copy link
Contributor

@FaysalSolutions FaysalSolutions commented Jun 7, 2023

  1. Match the color of the statusbar to the content
  2. Change the Icons of the statusbar to be more visible
  3. Disable the shadows of the app bar

So:

  1. In this pull request, I have set the size of the app bar on the LoginScreen to zero:

image

The status bar in Overview is changed when scrolling up or down.
The status bar in MyPICOS has the same color as the app bar.
Set the color of the StatusBar from LoginScreen.
Use SafeArea to enable changing the color of the StatusBar of MyPICOS from HomeScreen.
Delete the "appBarElevation" attribute from PicosScreenFrame.
Delete the "appBarElevation" parameter from QuestionnaireScreen.
@FaysalSolutions FaysalSolutions requested a review from a team June 7, 2023 13:56
@FaysalSolutions FaysalSolutions changed the title Change statusBarColor on the Overview and MyPICOS Change the color and visibility of the icons on the status bar for the Overview and MyPICOS Jun 7, 2023
Comment on lines 92 to 93
return SafeArea(
child: PicosScreenFrame(
Copy link
Contributor

Choose a reason for hiding this comment

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

PicosScreenFrame already includes a SafeArea so there are now technically two SafeArea's. Isn't there another way to solve this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved in 13a3e1d

Copy link
Contributor

Choose a reason for hiding this comment

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

There is a bug.
If you change to another screen without scrolling down the status bar continues to be white.

Comment on lines 45 to 46
final double appBarHeight = AppBar().preferredSize.height;
final double statusBarHeight = MediaQuery.of(context).padding.top;
Copy link
Contributor

Choose a reason for hiding this comment

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

As I understand _handleScroll gets executed for each little bit that get scrolled.
So if you scroll 1000 pixel these variables gets calculated 1000 times, but they never change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved in 815ddb2

Comment on lines 64 to 69
statusBarColor: _isScrolled
? Theme.of(context).extension<GlobalTheme>()!.darkGreen1
: Theme.of(context).bottomNavigationBarTheme.backgroundColor ??
Theme.of(context).canvasColor,
statusBarIconBrightness:
_isScrolled ? Brightness.light : Brightness.dark,
Copy link
Contributor

Choose a reason for hiding this comment

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

You check for _isScrolled twice in the same place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved in 61e017e


@override
Widget build(BuildContext context) {
final GlobalTheme theme = Theme.of(context).extension<GlobalTheme>()!;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you are using a stateful widget the theme variable can be turned into an static class attribute.

Comment on lines 48 to 51
setState(() {
_isScrolled =
_scrollController.offset >= (appBarHeight + statusBarHeight);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

setState is also called for every pixel scrolled.
Every time you call setState the widget gets rebuild which is very inefficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved in 4f3ab78

TextSpan(
text: AppLocalizations.of(context)!
.thankYouForParticipation,
_setSystemNavigationBarColor(context);
Copy link
Contributor

Choose a reason for hiding this comment

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

This gets executed on each widget rebuild. You should consider making this execute only once, since it never changes here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved in 25f3d72

Comment on lines +101 to +105
return SafeArea(
child: Scaffold(
appBar: PreferredSize(
preferredSize: const Size.fromHeight(0.0),
child: AppBar(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we have now multiple deviations from PicosScreenFrame it might be a good idea making PicosScreenFrame adjustable, so it can be used in more cases.

preferredSize: const Size.fromHeight(0.0),
child: AppBar(),
),
body: Center(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this Center widget can be removed.

children: <TextSpan>[
TextSpan(
text:
'${AppLocalizations.of(context)!.welcomeToPICOS},\n',
Copy link
Contributor

Choose a reason for hiding this comment

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

Try not to make lines above 80 characters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved in e9c442a

@FaysalSolutions FaysalSolutions deleted the branch healthcare-it-solutions:development June 30, 2023 08:24
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