-
Notifications
You must be signed in to change notification settings - Fork 12
Color and style standards #24
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
Open
Jignesh-dimagi
wants to merge
2
commits into
master
Choose a base branch
from
colors-style-standards
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -63,6 +63,25 @@ In short: “Crash early, fix quickly, and handle gracefully where it matters. | |
| - Use of Kotlin's built-in null safety features where necessary. | ||
| - Change Java files to Kotlin when Java-Kotlin intercompatibility introduces unnecessary and significant boilerplate code. | ||
|
|
||
| ### Color Resources (colors.xml) | ||
|
|
||
| - When adding a new color in `colors.xml`, define a base color named for the color itself (its hue or shade, e.g. `lavender_mist`, `slate_gray`), then define the feature/usage-specific color as a reference to that base color. | ||
| - Feature/usage colors should be named for where/how the color is used (e.g. `secondary_cta_btn_background`) and must reference a base color rather than a raw hex value. This keeps the palette reusable across features and decouples semantic usage from raw hex values. | ||
| - Example: | ||
| ```xml | ||
| <color name="lavender_mist">#D8D9F4</color> | ||
| <color name="secondary_cta_btn_background">@color/lavender_mist</color> | ||
| ``` | ||
| - Before adding a new base color, check `colors.xml` for an existing entry with the same hex value and reuse it instead of duplicating. | ||
|
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. We should add a note here, as I believe we want to avoid reusing colors with names that are unrelated to their intended use. |
||
| - When a color is used as part of a reusable UI element's styling, reference it from a style in `styles.xml` rather than setting it inline on the element (see [Styles](https://github.com/dimagi/commcare-android/blob/e7b31c0671a4fabb62f56da540ae6f033753b983/app/res/values/styles.xml#L231)). | ||
|
|
||
| ### Styles (styles.xml) | ||
|
|
||
| - When a new UI element (button, card, etc.) is created with specific styling requirements that need to be reused across the app, define a dedicated style for it in `styles.xml` rather than repeating attributes inline on each element. | ||
| - Apply the style to the element via `style="@style/YourStyleName"` so visual changes can be made in one place and stay consistent everywhere the element is used. | ||
| - Reference colors within styles using the named entries from `colors.xml` (e.g. `@color/secondary_cta_btn_background`) rather than raw hex values (see [Color Resources](https://github.com/dimagi/commcare-android/blob/e7b31c0671a4fabb62f56da540ae6f033753b983/app/res/values/colors.xml#L203)). | ||
| - For reference, see an existing reusable style in `styles.xml` (e.g. `CustomSecondaryCtaButtonStyle`). | ||
|
|
||
| ### Mobile Data Storage | ||
|
|
||
| - [Sqlite vs Shared Preferences](https://github.com/dimagi/open-source/blob/master/docs/mobile_storage_standards.md) | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Is this standard practice?
We could use
colors.xmlfor color names only (e.g.lavender_mist,slate_gray) and just reference those whenever needed in certain views, styles, etc. This seems like an unnecessary second step.For example, in the theoretical button we could use
@color/lavender_mistdirectly for the background color, which is self-documenting.Would love to hear thoughts from others on this as well
Uh oh!
There was an error while loading. Please reload this page.
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.
it's quite a standard practice but think more in context of styles and themes. Android itself provides color attributes like
primaryColorandsecondryColoretc. which is linked to the value from colors.xml in styles and themes.Agree thought it may not always be self sufficient. There is a need for the app to themify certain colors to reduce the overhead of thinking what color one should use for example for a button a dev is implementing that provides a secondry CTA. And in these cases we enrich the design languange of the app if we define a resource like -
name="secondary_cta_btn_background">@color/lavender_mist</color>instead of directly referencing thelavender_mistin the button.The other benefit of doing this is the scenario in which we need to change the secondry CTA color, instead of changing it all places in the xml, we can do so by just changing the value of
secondary_cta_btn_background.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.
@shubham1g5 Yeah that's the headspace I'm coming from (i.e. we should practice this in our themes and styles rather than directly in our
colors.xml). I think the earlier example I gave was a poor example.For example, say we define reusable themes in
themes.xml, and from there we defineprimaryColor,secondryColor, etc. inside a certain theme, referencing colors fromcolors.xmlsuch as@color/lavender_mist. The idea is that if we have a secondary CTA button, it pulls the background color directly form a theme resource attribute (or a style works too). Ideally, our Android app should derive all colors via themes or styles, so that in the future it's very easy to change our app's colors fromthemes.xml(orstyles.xml) rather than directly in an XML layout or a View.This also makes it very easy for us to implement color modes for the entire app in the future (e.g. light mode, dark mode, etc.)
Here's a very very rough example of what I'm talking about:
IMO, this is a much better way of enriching our design language
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.
Ultimately, I think our
colors.xmlshould only define base colorsThere 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.
yeah that checks with me. @Jignesh-dimagi Thoughts ?
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.
I agree. There are a few design tickets in the current and upcoming sprints where we should implement this approach. I think we should aim to create a style (may be reusable) for each/across individual component, such as the connect info card, the connect error message, and the connect progress or status card.
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.
I'm completely onboard with this. One concern I have is whether this would mean moving away from the app’s primary/secondary colors. I believe those should remain the source of truth across the different themes in the app.
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.
I agree as well
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.
We can define new theme/style attributes if necessary, but continue using the primary/secondary colors whenever possible
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.
@Jignesh-dimagi Are you able to iterate on this to incorpate our discussion in the thread