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

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

Merged
merged 5 commits into from
Feb 23, 2024

Conversation

saeedbashir
Copy link
Contributor

@saeedbashir saeedbashir commented Feb 9, 2024

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
Copy link
Contributor

rnr commented Feb 12, 2024

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
Copy link
Contributor Author

@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
Copy link

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
Copy link

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
Copy link
Contributor Author

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
Copy link

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
Copy link

moiz994 commented Feb 16, 2024

@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.

rnr
rnr previously approved these changes Feb 19, 2024
@@ -397,7 +398,7 @@ public struct AlertView: View {
lineJoin: .round,
miterLimit: 1
))
.foregroundColor(Theme.Colors.textPrimary)
.foregroundColor(Theme.Colors.secondardButtonBorderColor)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

secondard
Should it be secondary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo fixed.

@miankhalid miankhalid linked an issue Feb 20, 2024 that may be closed by this pull request
@volodymyr-chekyrta
Copy link
Contributor

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

@miankhalid
Copy link

All good @sdaitzman @marcotuts @moiz994 @ekangedx ?

@moiz994
Copy link

moiz994 commented Feb 23, 2024

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 saeedbashir merged commit ea0dfc2 into openedx:develop Feb 23, 2024
3 checks passed
@saeedbashir saeedbashir deleted the saeed/theming branch February 23, 2024 10:10
@saeedbashir
Copy link
Contributor Author

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.

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

Successfully merging this pull request may close these issues.

[iOS] Theming Customization Improvements
7 participants