search

openedx / openedx-app-ios

The mobile app for iOS for the Open EdX Platform.
Apache License 2.0
19 stars 13 forks source link

chore: enhancing app theme capability by changing adding colors #280

Closed saeedbashir closed 4 months ago

saeedbashir commented 4 months ago

This PR enhances the theme capability of the app by introducing more colors. And this PR generalizes primary, secondary, and disabled button styles for the whole app.

Changes For openedX Theme

While generalizing the button styles, the button styles will change at some places for the openedX theme. Other than the highlighted changes below nothing will change in openedX theme.

New Old

@sdaitzman Please review the changes mentioned-above for button changes.

rnr commented 4 months ago

This PR enhances the theme capability of the app by introducing more colors. And this PR generalizes primary, secondary, and disabled button styles for the whole app.

Changes For openedX Theme

While generalizing the button styles, the button styles will change at some places for the openedX theme. Other than the highlighted changes below nothing will change in openedX theme.

New Old @sdaitzman Please review the changes mentioned-above for button changes.

@saeedbashir Last two screens you show in the Description have unreadable button for Light theme for my opinion. All other looks good to me.

saeedbashir commented 4 months ago

@saeedbashir Last two screens you show in the Description have unreadable button for Light theme for my opinion. All other looks good to me.

That is a disbalanced button state. In the disabled state, the opacity/transparency is 30%, and in the active state, they will look like normal buttons.

sdaitzman commented 4 months ago

Hi @saeedbashir, thanks for tagging me for review! The first two screens look great to me. I'm okay with switching to using the accent color there.

Like @rnr mentioned, the buttons are hard to read in the final two screens. In those screenshots, the contrast ratio of ~1.14 is much too low. Even in the disabled state, users should be able to read the text to understand what action is not currently possible. I also think a grayed-out style is more clear for the disabled button state. Was there a design/usability rationale for switching to the transparent accent color for that button?

image

For reference, this tool can help check the contrast ratio against WCAG 2.1 values.

marcotuts commented 4 months ago

Are the images correct for before / after for the disabled buttons relating to the discussion examples? Both of those look the same to me

saeedbashir commented 4 months ago

Are the images correct for before / after for the disabled buttons relating to the discussion examples? Both of those look the same to me

@marcotuts They mixed up while updating the images. Corrected now.

sdaitzman commented 4 months ago

Copying from Slack, and adding a note about the second screen:

For the default OpenEdX mobile theme, we should continue using the grayed-out style inactive buttons (Figma component here) which are a bit more similar to mobile platform inactive button patterns. There are some pending updates on our end to adjust the color scheme and improve contrast ratios all around, currently working to document those changes in a new ticket.

Just noting here: both the EdX Paragon theme and the OpenEdX (grayed-out) inactive button style sometimes have a contrast ratio of ~2-3. This is still low, but in a better range than the inactive buttons currently proposed in this PR, and WCAG 2.2 allows it for inactive controls.

Lastly, I was wondering about the rationale for switching the "Leave" button in the second screen to use the accent color rather than the warning color. Since "Leave" is technically a destructive action (discards current changes), the use of the warning color there may make more sense.

moiz994 commented 4 months ago

@sdaitzman thanks for the feedback, Sam! I feel we can take up these edge cases in our design syncs better to discuss and address them in detail as it looks like it needs more attention. For now, let's move forward with the current PR to save time. As we are replicating edX's theme which is tried and tested, we have fewer risks going in with it. Once we finalize our design approach we can do a fast follow on this.

volodymyr-chekyrta commented 4 months ago

Can be merged if approved by the design/product side.

miankhalid commented 4 months ago

All good @sdaitzman @marcotuts @moiz994 @ekangedx ?

moiz994 commented 4 months ago

I think we're good to go with this. We discussed this in our design weekly this week as well. We decided to move forward with the PR for the sake of MVP but we will come back with more research and solid requirements to standardize the approach for buttons post-MVP. A solution was to add button types in the theme instead of colors on buttons but as mentioned a proper ticket will be created post-MVP.

saeedbashir commented 4 months ago

I think we're good to go with this. We discussed this in our design weekly this week as well. We decided to move forward with the PR for the sake of MVP but we will come back with more research and solid requirements to standardize the approach for buttons post-MVP. A solution was to add button types in the theme instead of colors on buttons but as mentioned a proper ticket will be created post-MVP.

Ok @moiz994 I've merged the PR.