-
-
Notifications
You must be signed in to change notification settings - Fork 50
CCCT-2843 Align The Chip, KPI And Progress Cards With The UI Kit #3937
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
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -432,7 +432,7 @@ | |
| <style name="Widget.CommCare.ConnectProgressCard" parent=""> | ||
| <item name="contentPrimaryColor">?attr/colorOnSurface</item> | ||
| <item name="contentAccentColor">?attr/colorPrimary</item> | ||
| <item name="contentDisabledColor">?attr/connectOutline</item> | ||
| <item name="contentDisabledColor">?attr/connectOnSurfaceEmphasis</item> | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "disabled" = "emphasis"
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| </style> | ||
|
|
||
| <style name="Widget.CommCare.ConnectSuccessFailureCard" parent=""> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,7 @@ import org.commcare.dalvik.databinding.ViewConnectInfoHalfCardBinding | |
| import org.commcare.views.extensions.bindOptional | ||
| import org.commcare.views.extensions.bindReservingSpace | ||
| import org.commcare.views.extensions.themeColor | ||
| import org.commcare.views.extensions.tint | ||
|
|
||
| /** | ||
| * Reusable half-width Connect info card. | ||
|
|
@@ -43,26 +44,32 @@ class ConnectInfoHalfCard | |
| get() = binding.infoCardSubtitleText.text | ||
| set(value) = binding.infoCardSubtitleText.bindReservingSpace(value) | ||
|
|
||
| /** When false the value reads as disabled, matching a disabled [ConnectProgressCard]. */ | ||
| /** When false the value and icon read as disabled, matching a disabled [ConnectProgressCard]. */ | ||
| var contentEnabled: Boolean = true | ||
| set(value) { | ||
| field = value | ||
| binding.infoCardValueText.setTextColor( | ||
| if (value) { | ||
| MaterialColors.getColor(this, com.google.android.material.R.attr.colorPrimary) | ||
| } else { | ||
| context.themeColor(R.attr.connectOnSurfaceDisabled) | ||
| }, | ||
| ) | ||
| applyContentColors() | ||
| } | ||
|
|
||
| var icon: Drawable? = null | ||
| set(value) { | ||
| field = value | ||
| binding.infoCardIcon.setImageDrawable(value) | ||
| binding.infoCardIcon.visibility = if (value == null) GONE else VISIBLE | ||
| applyContentColors() | ||
| } | ||
|
|
||
| private fun applyContentColors() { | ||
| val accent = | ||
| if (contentEnabled) { | ||
| MaterialColors.getColor(this, com.google.android.material.R.attr.colorPrimary) | ||
| } else { | ||
| context.themeColor(R.attr.connectOnSurfaceEmphasis) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "disabled" = "emphasis"
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| } | ||
| binding.infoCardValueText.setTextColor(accent) | ||
| binding.infoCardIcon.tint(accent) | ||
| } | ||
|
|
||
| var navigable: Boolean = false | ||
| set(value) { | ||
| field = value | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -248,7 +248,9 @@ class ConnectProgressCard | |
| binding.progressCardTitle.setTextColor(primary) | ||
| binding.progressCardBarLabel.setTextColor(primary) | ||
| binding.progressCardBarCount.setTextColor(accent) | ||
| binding.progressCardBarCaption.setTextColor(accent) | ||
| // The caption keeps the accent even when disabled - it explains why the card is | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: comment feels unnecessary
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done here |
||
| // blocked, so it stays legible rather than greying out with the figures. | ||
| binding.progressCardBarCaption.setTextColor(contentAccentColor) | ||
| binding.progressCardLinearBar.setProgressColor(accent) | ||
| binding.progressCardSemiCircle.progressColor = accent | ||
| binding.progressCardSemiCircle.valueTextColor = accent | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -472,7 +472,7 @@ class ConnectDeliveryDashboardFragmentTest { | |
| private fun accentColor(): Int = | ||
| MaterialColors.getColor(activity.findViewById(android.R.id.content), com.google.android.material.R.attr.colorPrimary) | ||
|
|
||
| private fun disabledColor(): Int = activity.themeColor(R.attr.connectOnSurfaceDisabled) | ||
| private fun disabledColor(): Int = activity.themeColor(R.attr.connectOnSurfaceEmphasis) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "disabled" = "emphasis"
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
|
||
| private fun progressResponse( | ||
| deliveries: List<String> = emptyList(), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,7 @@ import com.google.android.material.color.MaterialColors | |
| import org.commcare.CommCareTestApplication | ||
| import org.commcare.dalvik.R | ||
| import org.commcare.views.connect.ConnectProgressCard.State | ||
| import org.commcare.views.extensions.themeColor | ||
| import org.junit.Assert.assertEquals | ||
| import org.junit.Assert.assertTrue | ||
| import org.junit.Test | ||
|
|
@@ -173,12 +174,12 @@ class ConnectProgressCardTest { | |
| } | ||
|
|
||
| @Test | ||
| fun `contentEnabled recolors caption and semi-circle`() { | ||
| fun `contentEnabled recolors the semi-circle but leaves the caption at the accent`() { | ||
| val card = newCard() | ||
| val caption = card.findViewById<TextView>(R.id.progress_card_bar_caption) | ||
| val semi = card.findViewById<SemiCircleProgressBar>(R.id.progress_card_semi_circle) | ||
|
|
||
| val grey = ContextCompat.getColor(card.context, R.color.connect_grey) | ||
| val disabled = card.context.themeColor(R.attr.connectOnSurfaceEmphasis) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "disabled" = "emphasis"
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| val accent = MaterialColors.getColor(card, com.google.android.material.R.attr.colorPrimary) | ||
| val primary = ContextCompat.getColor(card.context, R.color.connect_text_color) | ||
|
|
||
|
|
@@ -189,9 +190,9 @@ class ConnectProgressCardTest { | |
| ) | ||
|
|
||
| card.bind(content.copy(contentEnabled = false)) | ||
| assertEquals(grey, caption.currentTextColor) | ||
| assertEquals(grey, semi.progressColor) | ||
| assertEquals(grey, semi.valueTextColor) | ||
| assertEquals(accent, caption.currentTextColor) | ||
| assertEquals(disabled, semi.progressColor) | ||
| assertEquals(disabled, semi.valueTextColor) | ||
| assertEquals(primary, semi.descriptionTextColor) | ||
|
|
||
| card.bind(content.copy(contentEnabled = true)) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if getting rid of this attribute is the right call? It seems like instead we could just map it to cool_gray_900 in the theme, rather than merge it with connectOnSurfaceEmphasis. And setting something called "disabled" to a value called "emphasis" at multiple places in the code feels confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, changed here