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.

Sign up for GitHub

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

Jump to bottom

AC-551: Implement Dark Mode (Experimental) , use CardView instead of custom Card and follow Material guidelines for applying themes. #618

Merged
merged 1 commit into from
Jul 17, 2019
Merged

AC-551: Implement Dark Mode (Experimental) , use CardView instead of custom Card and follow Material guidelines for applying themes. #618

merged 1 commit into from
Jul 17, 2019

Conversation

deepak140596
Copy link
Contributor

@deepak140596 deepak140596 commented Jul 14, 2019 •
edited
Loading

Description of what I changed

  • Added option to switch onto Dark Mode
  • Removed unnecessary use of @styles/AppTheme
  • Made use of @styles/AppThemeOrig in most of the Activities (it provides default Action Bar)
  • Removed unnecessary use of @drawable/card layout as it was incompatible with current material designs
  • Removed unnecessary use of setTextColor, setBackgroundColor, setBackgroundTheme, setBackground

Issue I worked on

JIRA Issue: https://issues.openmrs.org/browse/AC-551

Checklist: I completed these to help reviewers :)

  • My pull request only contains ONE single commit
    (the number above, next to the 'Commits' tab is 1).
  • I have added tests to cover my changes. (If you refactored
    existing code that was well tested you do not have to add tests)
  • All new and existing tests passed.
  • My pull request is based on the latest changes of the master branch.

@deepak140596 deepak140596 force-pushed the AC-551 branch 2 times, most recently from 509d29c to c3a2edd Compare July 14, 2019 10:50
f4ww4z
f4ww4z reviewed Jul 15, 2019
View reviewed changes
Copy link
Collaborator

@f4ww4z f4ww4z left a comment

Choose a reason for hiding this comment

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

@deepak140596 Seems that this PR is not just adding dark mode to the app. Rename the title accordingly.

openmrs-client/src/main/res/layout/list_visit_group.xml
@@ -12,55 +12,57 @@
~ Copyright (C) OpenMRS, LLC. All Rights Reserved.
-->

<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
<androidx.cardview.widget.CardView xmlns:android="http://schemas.android.com/apk/res/android"
Copy link
Collaborator

Choose a reason for hiding this comment

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

@deepak140596 What is the change here? Any screenshot of the improved list view?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @f4ww4z . I am posting the UI. All the rows are now using CardViews with ripple effect for onClick.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One issue still remains, we need to migrate the PNG/JPG icons to vectors. I will create a new issue for that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

One issue still remains, we need to migrate the PNG/JPG icons to vectors. I will create a new issue for that.

Noted, ping me there.

openmrs-client/src/main/res/layout/list_vital_group.xml
@@ -13,55 +12,61 @@
~ Copyright (C) OpenMRS, LLC. All Rights Reserved.
-->

<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
<androidx.cardview.widget.CardView xmlns:android="http://schemas.android.com/apk/res/android"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Show us a screenshot of this improved list view.

openmrs-client/src/main/res/values/color.xml Outdated Show resolved Hide resolved
openmrs-client/src/main/res/values/dimens.xml Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Jul 15, 2019 •
edited
Loading

Codecov Report

Merging #618 into master will decrease coverage by <.01%.
The diff coverage is 2.27%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #618      +/-   ##
=========================================
- Coverage    14.3%   14.3%   -0.01%     
=========================================
  Files         195     196       +1     
  Lines        9093    9104      +11     
  Branches      790     785       -5     
=========================================
+ Hits         1301    1302       +1     
- Misses       7713    7723      +10     
  Partials       79      79
Impacted Files Coverage Δ
...entrypatientlist/FormEntryPatientListActivity.java 0% <ø> (ø) ⬆️
...le/activities/formdisplay/FormDisplayActivity.java 0% <ø> (ø) ⬆️
...ivities/visitdashboard/VisitDashboardActivity.java 0% <ø> (ø) ⬆️
...ivities/addeditpatient/AddEditPatientActivity.java 0% <ø> (ø) ⬆️
...s/mobile/activities/formlist/FormListActivity.java 0% <ø> (ø) ⬆️
...mobile/activities/dialog/CustomFragmentDialog.java 0% <ø> (ø) ⬆️
...s/mobile/activities/settings/SettingsFragment.java 0% <0%> (ø) ⬆️
...cedpatients/SyncedPatientsRecyclerViewAdapter.java 0% <0%> (ø) ⬆️
...openmrs/mobile/utilities/ApplicationConstants.java 0% <0%> (ø) ⬆️
...mentrypatientlist/FormEntryPatientListAdapter.java 0% <0%> (ø) ⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d809cd7...b620f6d. Read the comment docs.

@deepak140596 deepak140596 changed the title AC-551: Implement Dark Mode (Experimental) AC-551: Implement Dark Mode (Experimental) and use CardView instead of custom Card and follow Material guidelines for applying themes. Jul 15, 2019
@deepak140596 deepak140596 changed the title AC-551: Implement Dark Mode (Experimental) and use CardView instead of custom Card and follow Material guidelines for applying themes. AC-551: Implement Dark Mode (Experimental) , use CardView instead of custom Card and follow Material guidelines for applying themes. Jul 15, 2019
@deepak140596
Copy link
Contributor Author

These are changed UI and for contrast, both the Day and Night UIs are posted.

Capture+_2019-07-15-17-34-03
Capture+_2019-07-15-17-35-22
Capture+_2019-07-15-17-36-06
Capture+_2019-07-15-17-36-15
Capture+_2019-07-15-17-36-42
Capture+_2019-07-15-17-36-58
Capture+_2019-07-15-17-38-19
Capture+_2019-07-15-17-38-37
Capture+_2019-07-15-17-39-05
Capture+_2019-07-15-17-39-11
Capture+_2019-07-15-17-39-17
Capture+_2019-07-15-17-39-23
Capture+_2019-07-15-17-39-36
Capture+_2019-07-15-17-39-50
Capture+_2019-07-15-17-40-02

@f4ww4z
Copy link
Collaborator

f4ww4z commented Jul 16, 2019

@deepak140596 You'll need to implement dark mode in 'Add Patients' screen of 'Find Patients' section. See the screenshot:
image

Also, try full white (#FFFFFF) for all text color when in dark mode, I think it will look better.

@deepak140596
Copy link
Contributor Author

@f4ww4z , I haven't used any hardcoded text color. Android is deciding the text color. We have two options for setting text colors: i) not using any attributes for text color ii) using text color as
android:textColor="?android:attr/textColorPrimary".
The second option will make the text full black (#000000) in Light Mode and full white (#ffffff) in Dark Mode. This needs to be specifically set and should be used in headings.

For body we should just leave it as it is. In the same example we can see there are 2 shades of white.

One exception though, I have used a neutral green color for "Unable to login" option to highlight. Should I replace the color to White?

f4ww4z
f4ww4z approved these changes Jul 17, 2019
View reviewed changes
Copy link
Collaborator

@f4ww4z f4ww4z left a comment

Choose a reason for hiding this comment

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

@deepak140596 Looks good now!

Where does the 'unable to login' view show up?

@f4ww4z f4ww4z merged commit 8f5fc67 into openmrs:master Jul 17, 2019
@deepak140596 deepak140596 deleted the AC-551 branch July 21, 2019 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@f4ww4z f4ww4z f4ww4z approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@deepak140596 @codecov-io @f4ww4z