google / android-fhir

The Android FHIR SDK is a set of Kotlin libraries for building offline-capable, mobile-first healthcare applications using the HL7® FHIR® standard on Android.
https://google.github.io/android-fhir/
Apache License 2.0
465 stars 245 forks source link

Read only view in questions/forms are too light. #939

Closed santosh-pingle closed 2 years ago

santosh-pingle commented 2 years ago

IMPORTANT: All PRs must be linked to an issue (except for extremely trivial and straightforward changes).

Issue #917

Description

Alternative(s) considered Have you considered any alternatives? And if so, why have you chosen the approach in this PR?

Type Choose one: Feature

Screenshots (if applicable)

Before implementation Edit text with Mark value

Screen Shot 2021-11-24 at 3 21 36 PM

After implementation Edit text with Mark value

Screen Shot 2021-11-24 at 3 24 29 PM

Checklist

santosh-pingle commented 2 years ago

@jing could you please confirm proposed implementation?

codecov[bot] commented 2 years ago

Codecov Report

Merging #939 (f6635cb) into master (a0e522e) will decrease coverage by 0.03%. The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #939      +/-   ##
============================================
- Coverage     88.33%   88.29%   -0.04%     
  Complexity      491      491              
============================================
  Files           111      111              
  Lines          9335     9340       +5     
  Branches        636      641       +5     
============================================
+ Hits           8246     8247       +1     
+ Misses          751      750       -1     
- Partials        338      343       +5     
Impacted Files Coverage Δ
.../QuestionnaireItemAutoCompleteViewHolderFactory.kt 26.89% <0.00%> (-0.19%) :arrow_down:
...ws/QuestionnaireItemDatePickerViewHolderFactory.kt 35.71% <0.00%> (-0.52%) :arrow_down:
...uestionnaireItemDateTimePickerViewHolderFactory.kt 22.22% <0.00%> (-0.39%) :arrow_down:
...iews/QuestionnaireItemEditTextViewHolderFactory.kt 45.45% <0.00%> (-1.43%) :arrow_down:
...va/com/google/android/fhir/db/impl/DatabaseImpl.kt 90.12% <0.00%> (+1.23%) :arrow_up:

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 a0e522e...f6635cb. Read the comment docs.

jingtang10 commented 2 years ago

please also update the PR name and description

santosh-pingle commented 2 years ago

Updating code changes as per review comments.

jingtang10 commented 2 years ago

please mark as ready to review if ready. thanks!

santosh-pingle commented 2 years ago

@jingtang10 Implemented contrast change is only visible to questionnaire edit text view (QuestionnaireItemEditTextViewHolderFactory) and not for others. Do we need to apply it to other questionnaire views?

jingtang10 commented 2 years ago

@jingtang10 Implemented contrast change is only visible to questionnaire edit text view (QuestionnaireItemEditTextViewHolderFactory) and not for others. Do we need to apply it to other questionnaire views?

yeah you do. anything with the edit text you'll need to apply the stroke color, anything with text you'll probably need to apply the text color.

jingtang10 commented 2 years ago

have you also tested this with dark theme? please attach some screenshots. thanks!

jingtang10 commented 2 years ago

also please attach before/after screenshots with other widgets (e.g. radio button group, checkbox group) if you do them in the same PR.

santosh-pingle commented 2 years ago

have you also tested this with dark theme? please attach some screenshots. thanks!

looking into it.

santosh-pingle commented 2 years ago

have you also tested this with dark theme? please attach some screenshots. thanks!

looking into it.

@shelaghm when #9E9E9E get applied to the disabled text and container it works as expected in light theme (please refer earlier screenshots) but when it switch to the dark theme then contrast/opacity is too high. To fix this issue, it is required to use alpha value and theme attribute to create color state list resource. As you mentioned 38% opacity in earlier comments I assumed alpha value is .38 as range is 0 to 1, now it works in both themes, but if you think alpha value can be greater than .38 please let me know.

Light theme : read only view with alpha .38

light theme alpha  38

Dark theme : readonly view with alpha .38

dark theme with alpha  38

Just for reference text color with alpha .45 and container border stroke color with alpha .38

light theme alpha  45
santosh-pingle commented 2 years ago

Q.4 readonly autocomplete textview, light theme, alpha .38

light theme autocomplete  38

Q.4 readonly autocomplete textview, dark theme, alpha .38

dark theme autocomplete  38
santosh-pingle commented 2 years ago

Q Hep B y / n readonly view QuestionnaireItemBooleanTypePickerViewHolderFactory, alpha .38, light theme

light boolean  38

Q Hep B y / n readonly view QuestionnaireItemBooleanTypePickerViewHolderFactory, alpha .38, dark theme

dark boolean  38
santosh-pingle commented 2 years ago

Q. 1st dose 2nd dose readonly view QuestionnaireItemDateTimePickerViewHolderFactory, light theme, alpha .38

datetime picker light alpha  38

Q. 1st dose 2nd dose readonly view QuestionnaireItemDateTimePickerViewHolderFactory, dark theme, alpha .38

datetimepicker dark alpha  38
santosh-pingle commented 2 years ago

Q.1 & Q.3 readonly view QuestionnaireItemDialogSelectViewHolderFactory, light theme, alpha .38

light drop down dialog select alpha  38

Q.1 & Q.3 readonly view QuestionnaireItemDialogSelectViewHolderFactory, dark theme, alpha .38

dark drop down dialog select alpha  38
santosh-pingle commented 2 years ago

@jingtang10 Can you please review it? Thanks!

santosh-pingle commented 2 years ago

readonly view QuestionnaireItemDropDownViewHolderFactory alpha .38

Screen Shot 2021-12-03 at 4 47 41 PM Screen Shot 2021-12-03 at 4 48 26 PM
santosh-pingle commented 2 years ago

readonly view QuestionnaireItemCheckBoxGroupViewHolderFactory alpha .38

Screen Shot 2021-12-03 at 4 47 20 PM Screen Shot 2021-12-03 at 4 46 44 PM
anita-vadverao commented 2 years ago

Retested "Readonly" changes by adding "readOnly" : true" attribute in Questionnaire JSON and it is properly visible after adding contrast to text. Working as expected.