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

Issue/10433 semantic color names pt2 #10479

Closed
wants to merge 4 commits into from

Conversation

khaykov
Copy link
Member

@khaykov khaykov commented Sep 4, 2019

Part of #10433

This PR is intended to be a part of a gradual effort to get our styling in order and reduce tech debt.

In this PR I switched three major text color we are using to their semantic attributes.

Semantic Attribute Name Semantic color name (color studio name) iOS name
wpColorTextTetriary neutral_40 (gray_40) textTetriary
wpColorTextQuaternary neutral_30 (gray_30) textQuaternary
wpColorTextInverted @ android:color/white textInverted

I left out changes in selectors and some parts where we set the color in code, until more colors are converted.

Most of the changes in layout are pretty straightforward and do not require much testing. I would direct attention to the changes in parts that cover programmatic color changes.

To test:

  • Look around different areas of the app to make sure nothing is crashing and colors look ok.

Update release notes:

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@khaykov khaykov added this to the 13.3 milestone Sep 4, 2019
@khaykov khaykov requested a review from theck13 September 4, 2019 05:15
@peril-wordpress-mobile
Copy link

You can test the changes on this Pull Request by downloading the APK here.

@theck13 theck13 self-assigned this Sep 4, 2019
@theck13
Copy link
Contributor

theck13 commented Sep 4, 2019

Look around different areas of the app to make sure nothing is crashing and colors look ok.

Can we include screenshots or specify which areas of the app to check?

@khaykov
Copy link
Member Author

khaykov commented Sep 4, 2019

@theck13 Hm. Changes are all around the place, so I wonder, is there a better way to approach this. You had some large scale color-related PR's before, any ideas on how we can improve this? :) Maybe include more color changes in one PR, so we don't have to go through whole app again?

@theck13
Copy link
Contributor

theck13 commented Sep 4, 2019

When we made the app-wide color changes in #9495, a designer and I reviewed every screen in the app before creating the pull request so that the reviewing developer would only need to focus on the code and not need to verify every color was correct. When we updated drawables in the app with tint, I created multiple pull requests like #8881 with specific steps to test the screen(s) affected by the changes. Both of those methods require a lot of work before creating the pull request, but they ensure a much easier review process.

In the case of semantic color changes, we may have to do some of both. If the color of any interface elements are changed, we should include a designer and any screenshots showing the differences. If we can limit the updates to a screen or two at a time, that would make the test steps easier to write and review. That may involve adding multiple entries to the attrs.xml file at one time and not necessarily changing all associated color references to use semantic references at once, but that would make the review process much quicker for designer and developer.

@khaykov
Copy link
Member Author

khaykov commented Sep 9, 2019

Closing in favor of #10496

@khaykov khaykov closed this Sep 9, 2019
@theck13 theck13 removed their assignment Sep 9, 2019
@khaykov khaykov deleted the issue/10433-semantic-color-names-pt2 branch April 15, 2020 16:58
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.

2 participants