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

Introduce helpers for theme colors and document how to support light and dark themes #2178

Merged
merged 7 commits into from
May 17, 2018

Conversation

shobhitagarwal1612
Copy link
Contributor

@shobhitagarwal1612 shobhitagarwal1612 commented May 9, 2018

Closes #2165

What has been done to verify that this works as intended?

Verified manually on Moto G5 plus Android 7.0

Why is this the best possible solution? Were any other approaches considered?

This adds helper functions for getting theme colors instead of fetching using the attribute id and also create a new custom attribute iconColor to be used in place of coloControlNormal for better clarity

Are there any risks to merging this code? If so, what are they?

Yes, refactoring is always risky. The color of icons might behave differently if the new attribute is not applied to all places

Do we need any specific form for testing your changes? If so, please attach one.

No

Does this change require updates to documentation? If so, please file an issue at Here and include the link below.

Before submitting this PR, please make sure you have:

  • run ./gradlew pmd checkstyle lint findbugs and confirmed all checks still pass.
  • verified that any new UI elements use theme colors so that they work with both light and dark themes.

@lognaturel
Copy link
Member

Looks good to me! Thanks, @shobhitagarwal1612.

@lognaturel lognaturel mentioned this pull request May 14, 2018
2 tasks
@lognaturel lognaturel changed the title Adds description on how to support both themes when adding new UI components Introduce helpers for theme colors and document how to support light and dark themes May 15, 2018
@lognaturel
Copy link
Member

@shobhitagarwal1612 something has happened to the history. It looks like you may have tried to rebase on master to fix conflicts but maybe ended up with an older version? Only the relevant commits should show up in the diff.

@shobhitagarwal1612
Copy link
Contributor Author

I rebased onto my origin master branch instead of upstream master branch which wasn't updated in quite some time

@lognaturel
Copy link
Member

Ahh, much better! Hope it wasn't too painful. Now we're back in business. 😊

@shobhitagarwal1612
Copy link
Contributor Author

Just had to rebase and then drop some commits to fix it

@mmarciniak90
Copy link
Contributor

Tested with success

I did not notice regression.
I'm just not sure about Save icon in Import/Export Settings view. There is a crash (fixed in #2192).
I will be able to check that after the merge.
Verified on Android: 4.1, 4.2, 4.4, 5.1, 6.0, 8.1

@opendatakit-bot unlabel "needs testing"
@opendatakit-bot label "behavior verified"

@shobhitagarwal1612
Copy link
Contributor Author

@mmarciniak90 I have updated the branch with the fix so the remaining task can be tested now

@lognaturel
Copy link
Member

The buttons are looking good so I'll merge and there will be more verification with a second beta and pre-release testing. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants