-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Feature/awesome accessibility color audit #7642
Feature/awesome accessibility color audit #7642
Conversation
…id into feature/awesome-accessibility-color-audit
Great work here, and thanks for surfacing the inconsistencies 😄 There are a LOT of little changes here, so making changes throughout the entire app will mean a substantial shift in feel. My general take is that darkening text and icons slightly is a good thing, but it's a fairly substantial change and we will have to adjust our design docs (not a bad thing!) and other things, so I want to make sure we have wide design approval. For that reason: cc @folletto @mattmiklic @SylvesterWilmott Also, I'd like to see if we can/should match similar on iOS. cc @etoledom With all that said, here are some notes on your screenshots:
|
Note that these screens aren't just low contrast, it's also an old design. If they were designed today they would be likely on cards or at least white background.
Note: contrast of decorative icons doesn't matter. The only time it matters when an icon contrast has to be ok is when the icon has no label on screen. So, to answer: no need to change these.
Reader in Calypso doesn't follow our guidelines (long discussion), so I wouldn't refer to Reader for design patterns guidance.
For confirmation: that's a good variable to use, like Calypso's
Absolutely yes. As a geneal consideration here: I'm totally fine in favoring a darker contrast in these old screens as any other alignment work we'd have to do would have to start by fixing the general design patterns first. ;) |
@folletto thanks for the insight! What I also mean by getting our style game together is standartizing and developing a proper style sheet, so we won't have to manually set a color or a text size to elements anymore. Looking at both android app and calypso I see that there is an affort to achieve this, but it's quite localized :) @iamthomasbishop Thanks for a great review! After looking at all these greys I stopped noticing things :D I fixed most of the problems, and also have a couple of comments:
I found that
SEND button is always active, on Android at least.
We are using
You mean the color of comment/like icons/labels? If so, we have a ton of different greys :) I was going for the same color as the rest of the icons (like in My Site tab). Would you like me to change the color to something else?
I looked at the Calypso for reference on this one. This is how the same notification looks there: Shoul we go for a parity with Calypso or it doesn't really matter? (maybe @folletto has some thoughts on this one too). |
Interesting, reading some of the code it looked like to me we already had variables and so on, but if not, I agree that's a worthy effort. :) |
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.
Wooooow, what a huge amount of work you've done, great job!!! Fixing inconsistencies between different preferences screens makes me really happy :-).
I've tried to go through most of the screens and I've found just few issues.
- The switch background color seems broken.
- My Site -> Comments screen seems to be a bit inconsistent with Notifications screen.
PPPPS: We need to get our style game together.
100 % Agree! IMO we shouldn't use specific colors (such as grey_dark
) in our layouts -> we should either use something general like hint_primary
or use specific colors only in styles.xml and themes.xml. Using specific colors in layouts makes it really difficult to change the look of our application as the same color is used for different UI components..... But this is definitely out of scope of this PR.
Thanks!!!
@@ -229,6 +229,7 @@ public boolean onPreferenceChange(final Preference preference, final Object valu | |||
int index = Integer.parseInt(value.toString()); | |||
CharSequence[] entries = editorTypePreference.getEntries(); | |||
editorTypePreference.setSummary(entries[index]); | |||
editorTypePreference.setValue(value.toString()); |
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 line really needed? As far as I understand, the onPreferenceChange is called before the system updates the value - if we return true from the onPreferenceChange, the system should update the value automatically.
I'm just a bit afraid, that updating the value manually might have some side effects. WDYT?
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.
Yes, you are right, the system will save preference, but we are using custom DetailListPreference
(for style reasons) which will not pick the change automatically so we have to manually set the value.
This is nice, the inconsistency on Android has been a pet peeve of mine for a while but there just hadn't been time yet to really audit how we're using color to make it more consistent. Big 👍 to this effort. It would be helpful if there were an APK the designers could install to give this a test-run to check for edge cases that might need special attention, like the blue-on-blue toggle switch that @malinajirka noted above. |
@iamthomasbishop I we absolutely should! We should think about Android and iOS apps as one consistent product because... aren't they? I haven't seen any big inconsistency on iOS but there are some low contrast details for sure. It might be good to finish this changes on Android, and then apply the final result on iOS. |
@khaykov Again, great work! I'm so happy that we're focused on consistency and accessibility. Regarding your last comments above: Placeholder text color (gray-darken-10) looks good 👍
Fair enough, let's roll w/ it.
👍
Sounds good - let's roll w/ it :)
Yea, that's fine - let's match Calypso there. Also good catches from @malinajirka . I'm not exactly sure what we should do about the blue-on-blue toggle...I'm not seeing the same thing if I go to
@etoledom - I'm not super concerned with every single color of every component matching exactly across platforms (yet! A new design language will give us the opportunity to get all of our ducks in a row across all platforms - so we will be addressing that in a wider way soon!). With that said, I do think within each app colors of components should be consistent (and contrast-tested, of course) across all screens within the app. |
@iamthomasbishop thanks! If you notice any weird colors after we merge this PR let me know and I'll fix them asap :) @malinajirka I think I fixed all the issues you mentioned and ready for another round :) |
Thanks @khaykov for fixing all the issues;)! I have found few more issues. I'm sorry I missed them during the first round:(. 3., 4., 5. These might still have contrast issues Thanks!!! |
…id into feature/awesome-accessibility-color-audit # Conflicts: # WordPress/src/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsFragment.java # WordPress/src/main/res/xml/notifications_settings.xml
@malinajirka thank you so much for taking a look! I updated contrast at Sharing preferences screen, but I'll leave the overall design for another issue (#7682) - it's custom Preference screen, and redesigning it is probably will require some consideration. The User Detail screen is actually supposed to be like this - we are reducing the opacity of the container to indicate that it's disabled. |
Thank you so much for this PR. Looks good! |
Accessibility color audit turned into Calypsofication of colors.
I was surprised to learn that our styles and colors are total mess, specifically preference screens. In our app we have preferences that look like this :
With dialogs that looks like this:
So apart from adressing color contrast issues I tried to adress inconsistencies like that.
Now, there are parts where I wasn't exatly sure what colors to use. Reader in calypso, for example, uses low contrast icons, contrary to the rest of the interface, so I had to improvise.
So, I need a help of a designer with a sharp eye, like @iamthomasbishop, to help me navigate this 50 shades of grey ;)
I compiled before-after screenshots of major changes, so please let me know if you feel like we can use any other colors at any element.
Part 1
Part 2
Part 3
Part 4
Part 5
I haven't touched the login icons color, as there is no direct equivalent in Calypso. There are no contrast issues there, apart from icons. Should I change thir color to something greyer?
PS: Accessibility monitor is still angry about contrast of some colors, but I did my best to bring it at least above 3 (?).
PPS: Ringtone prefference is using system colors.
PPPS: I probably missed some colors.
PPPPS: We need to get our style game together.