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

added dark mode option #40

Merged
merged 35 commits into from
Jan 5, 2021

Conversation

PseudoCode01
Copy link
Contributor

With an attempt to make app more attractive I have added the dark mode to the app using theme provider since in today's time everyone like the dark mode .

Preview :

circuitversedarkmode

ScreenShots
Screenshot_20201207_142613
Screenshot_20201207_142637
Screenshot_20201207_142719

@PseudoCode01 PseudoCode01 marked this pull request as ready for review December 7, 2020 09:35
@@ -3,4 +3,4 @@ distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
zipStoreBase=GRADLE_USER_HOME
zipStorePath=wrapper/dists
distributionUrl=https\://services.gradle.org/distributions/gradle-5.6.2-all.zip
distributionUrl=https://services.gradle.org/distributions/gradle-6.0.1-all.zip
Copy link
Member

Choose a reason for hiding this comment

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

Does the changes require Gradle update from 5.6.2 to 6.0.1 ?

lib/main.dart Outdated
AppTheme(
id: 'dark',
data: ThemeData(
primaryColor: Colors.grey[900],
Copy link
Member

Choose a reason for hiding this comment

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

Would be better, if we keep the colors for Dark Mode in app_theme for reusability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok i will put in app_theme

@@ -1,7 +1,7 @@
import 'package:flutter/material.dart';

class AppTheme {
AppTheme._();
class PrimaryAppTheme {

This comment was marked as resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have used theme provider dependency whose AppTheme keyword was conflicting with the AppTheme class.

Copy link
Member

Choose a reason for hiding this comment

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

Can we use CVTheme or CircuitVerseTheme ? PrimaryTheme doesn't go well with the dark colours imo!

lib/ui/components/cv_header.dart Outdated Show resolved Hide resolved
@manjotsidhu

This comment has been minimized.

@jaggu21
Copy link
Contributor

jaggu21 commented Dec 7, 2020

Adding on to @manjotsidhu 's suggestions, I think it gives better user experience if he/she can toggle between light and dark mode at any location from the app.

Say that you're on Add Assignments page. You cannot toggle between light and dark modes on this page(since app drawer is not accessible), and the user will have to pop the pages from stack to reach groups page and only then the user will be able to app drawer to toggle between the two themes.

@PseudoCode01
Copy link
Contributor Author

Yes , I am working on all the changes mentioned

@PseudoCode01
Copy link
Contributor Author

I have made the asked changes

@manjotsidhu
Copy link
Member

I have made the asked changes

@iCoder-007 can you send screenshots for only new changes for others to review ?

@manjotsidhu
Copy link
Member

Also please fix formatting's using flutter format .

@PseudoCode01
Copy link
Contributor Author

Screenshots

Screenshot_20201208_162709
Screenshot_20201208_162735
Screenshot_20201208_162812

@manjotsidhu
Copy link
Member

I am not sure if the FAB button in home is a good idea or not. I think @Nitish145 @jaggu21 should give the inputs on this

@PseudoCode01
Copy link
Contributor Author

if you want i can place it in appbar at right

@jaggu21
Copy link
Contributor

jaggu21 commented Dec 8, 2020

I agree with @manjotsidhu . Have a look at the following screenshot.
FAB
FAB's color and the background color match which slightly looks off to me.

Apart from that few minor issues:
1)Changing the color of arrow to white would make it look better than keeping it as black.
arrow

  1. It looks better if we follow same theme throughout the entire app. Also on switching from light mode to dark mode ,the lowermost section of the HTML editor changes it's color(from grey to green) while the topmost section remains the same(grey). I think it would be better if both either change or both remain the same. Though I am not sure if this is major issue.
    t1

@PseudoCode01
Copy link
Contributor Author

so should i remove fab button and place it in appbar right
and the rest i will change to as you recommended .

@PseudoCode01
Copy link
Contributor Author

As asked i have made the changes please have a look

@manjotsidhu
Copy link
Member

@iCoder-007 looks good to me, kindly run flutter format . to format the documents.

@PseudoCode01
Copy link
Contributor Author

I have already done that but everything remains unchanged

@manjotsidhu
Copy link
Member

I have already done that but everything remains unchanged

Here the logs say that lib\ui\views\projects\components\project_card.dart is not formatted
https://github.com/CircuitVerse/mobile-app/pull/40/checks?check_run_id=1523553695#step:6:87

@manjotsidhu

This comment has been minimized.

@PseudoCode01
Copy link
Contributor Author

when i am running flutter format . it shows unchanged
and the improvements you mentioned i am fixing them

@manjotsidhu
Copy link
Member

when i am running flutter format . it shows unchanged

It will show Unchanged for all files except project_card.dart and it will format that.

@PseudoCode01
Copy link
Contributor Author

have a look

@manjotsidhu
Copy link
Member

Here is the diff of the changes while formatting :

Diff (Click to expand)
diff --git a/lib/ui/views/projects/components/project_card.dart b/lib/ui/views/projects/components/project_card.dart
index 5bf414d..1a78ed7 100644
--- a/lib/ui/views/projects/components/project_card.dart
+++ b/lib/ui/views/projects/components/project_card.dart
@@ -56,8 +56,12 @@ class _ProjectCardState extends State<ProjectCard> {
               textAlign: TextAlign.center,
               style: Theme.of(context).textTheme.headline5.copyWith(
                     color: ThemeProvider.themeOf(context).id == 'dark'
-                        ? widget.isHeaderFilled ? Colors.black : Colors.white
-                        : widget.isHeaderFilled ? Colors.white : Colors.black,
+                        ? widget.isHeaderFilled
+                            ? Colors.black
+                            : Colors.white
+                        : widget.isHeaderFilled
+                            ? Colors.white
+                            : Colors.black,
:...skipping...
diff --git a/lib/ui/views/projects/components/project_card.dart b/lib/ui/views/projects/components/project_card.dart
index 5bf414d..1a78ed7 100644
--- a/lib/ui/views/projects/components/project_card.dart
+++ b/lib/ui/views/projects/components/project_card.dart
@@ -56,8 +56,12 @@ class _ProjectCardState extends State<ProjectCard> {
               textAlign: TextAlign.center,
               style: Theme.of(context).textTheme.headline5.copyWith(
                     color: ThemeProvider.themeOf(context).id == 'dark'
-                        ? widget.isHeaderFilled ? Colors.black : Colors.white
-                        : widget.isHeaderFilled ? Colors.white : Colors.black,
+                        ? widget.isHeaderFilled
+                            ? Colors.black
+                            : Colors.white
+                        : widget.isHeaderFilled
+                            ? Colors.white     
+                            : Colors.black,    
                   ),
             ),
           ),

@manjotsidhu
Copy link
Member

have a look

Looks like either your flutter version is old or you have set your line length more than the default.
image

@PseudoCode01
Copy link
Contributor Author

have a look at changes

@manjotsidhu
Copy link
Member

@iCoder-007 we recently merged #37 and you might need to fix build errors and text colors for Dark Mode in Featured Project Card.

@PseudoCode01
Copy link
Contributor Author

Please check

@@ -3,4 +3,4 @@ distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
zipStoreBase=GRADLE_USER_HOME
zipStorePath=wrapper/dists
distributionUrl=https\://services.gradle.org/distributions/gradle-5.6.2-all.zip
distributionUrl=https\://services.gradle.org/distributions/gradle-5.6.2-all.zip
Copy link
Member

Choose a reason for hiding this comment

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

@iCoder-007

@Nitish145
Copy link
Member

@iCoder-007 can u please undo the last merge commit, and try rebasing to fix the conflicts

@PseudoCode01
Copy link
Contributor Author

PseudoCode01 commented Dec 23, 2020

i saw that that there are some new changes in master branch so i have pulled the changes and will fix the conflicts manually , will this work

@Nitish145
Copy link
Member

Yes

@PseudoCode01
Copy link
Contributor Author

I think due to latest merge the ui test are failing

@PseudoCode01
Copy link
Contributor Author

And sorry for the delays I was quiet busy

@manjotsidhu
Copy link
Member

@iCoder-007 Have you merged d60561b ?

@manjotsidhu
Copy link
Member

@iCoder-007 try pushing again, the CI will pass

@PseudoCode01
Copy link
Contributor Author

okk

Copy link
Member

@Nitish145 Nitish145 left a comment

Choose a reason for hiding this comment

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

Please make the requested changes.

@manjotsidhu
Copy link
Member

@iCoder-007 Can you check if CVTypeAheadField which was merged is following the dark theme as well ?

@PseudoCode01
Copy link
Contributor Author

@iCoder-007 Can you check if CVTypeAheadField which was merged is following the dark theme as well ?

yes, and sorry for delay

@manjotsidhu
Copy link
Member

@Nitish145

@PseudoCode01
Copy link
Contributor Author

please check

@manjotsidhu
Copy link
Member

@tachyons Seems like @Nitish145 is busy, can you merge this ?

@Nitish145 Nitish145 merged commit c22c15b into CircuitVerse:master Jan 5, 2021
@manjotsidhu manjotsidhu mentioned this pull request Jan 5, 2021
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.

5 participants