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: design team feedback to improve app theming capability #365

Merged

Conversation

saeedbashir
Copy link
Contributor

@saeedbashir saeedbashir commented Mar 22, 2024

This PR improves the theme capability of the app by introducing more color options. And also, a custom placeholder is added to all the input fields. By adding the custom placeholder, it will help in meeting the a11y requirements.

Note: The rounded and rectangular corners are configurable, via config files.

Visually, the following changes are made to the openedX theme:

Old Screen New Screen

Copy link
Contributor

@rnr rnr left a comment

Choose a reason for hiding this comment

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

LGTM
Found one design mistake:
Screenshot 2024-03-26 at 15 58 27

rnr
rnr previously approved these changes Mar 27, 2024
@volodymyr-chekyrta
Copy link
Contributor

My understanding from yesterday's weekly design meeting is that these changes are still not approved, so they can't be merged.
cc @moiz994 @ekangedx @sdaitzman @marcotuts

@moiz994
Copy link

moiz994 commented Mar 28, 2024

@volodymyr-chekyrta I believe the design feedback was to bring back the subtitle on the sign-in and register screens. The rest are just theme-based changes that will not affect the open edX build.

The new copy of the subtitles is as follows:

Sign-in screen:
"Welcome back! Sign in to access your courses."

Register screen:
"Create an account to start learning today!"

Let me know what you think.

@marcotuts @sdaitzman @ekangedx what do you guys think of the new subtitles?

@sdaitzman
Copy link

Those new subtitles make sense to me. They're more friendly/welcoming/natural vs. the old copy there.

What are the other changes in this PR?

  • Adding a theming option to eliminate border radius on buttons and text fields, with the default staying the same (rounded)
  • Adding a custom placeholder to all input fields: is this shown in the screenshots above? Is it to support different builds having different placeholder text for some fields?

Are there any other changes I'm missing?

@saeedbashir
Copy link
Contributor Author

Adding a theming option to eliminate border radius on buttons and text fields, with the default staying the same (rounded)

This is already implemented and part of the current codebase.

Adding a custom placeholder to all input fields: is this shown in the screenshots above? Is it to support different builds having different placeholder text for some fields?

@sdaitzman This PR adds support for configuring, input text fields, background color, placeholder color, and input text color. The placeholder texts will remain the same for the open edX community.

I've added this capability the input background colors for open edX are different for light and dark modes, but for edX its same.

@saeedbashir
Copy link
Contributor Author

My understanding from yesterday's weekly design meeting is that these changes are still not approved, so they can't be merged. cc @moiz994 @ekangedx @sdaitzman @marcotuts

@volodymyr-chekyrta the subtitles are reverted back and updated with latest text. It's ready for review.

1 similar comment
@saeedbashir
Copy link
Contributor Author

My understanding from yesterday's weekly design meeting is that these changes are still not approved, so they can't be merged. cc @moiz994 @ekangedx @sdaitzman @marcotuts

@volodymyr-chekyrta the subtitles are reverted back and updated with latest text. It's ready for review.

@volodymyr-chekyrta
Copy link
Contributor

My understanding from yesterday's weekly design meeting is that these changes are still not approved, so they can't be merged. cc @moiz994 @ekangedx @sdaitzman @marcotuts

@volodymyr-chekyrta the subtitles are reverted back and updated with latest text. It's ready for review.

@saeedbashir thank you! 🚀

Copy link
Contributor

@volodymyr-chekyrta volodymyr-chekyrta left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@volodymyr-chekyrta
Copy link
Contributor

@saeedbashir, could you please fix the lint commit error before merge?

@saeedbashir
Copy link
Contributor Author

@saeedbashir, could you please fix the lint commit error before merge?

It's happening because of merge branch message and it's bit hard to reword that message, anyhow it will be fixed in squash while merging the PR.

@volodymyr-chekyrta
Copy link
Contributor

@saeedbashir, could you please fix the lint commit error before merge?

It's happening because of merge branch message and it's bit hard to reword that message, anyhow it will be fixed in squash while merging the PR.

👍

@volodymyr-chekyrta volodymyr-chekyrta merged commit 546e6d7 into openedx:develop Mar 29, 2024
2 of 3 checks passed
@saeedbashir saeedbashir deleted the saeed/design_team_feedback branch March 29, 2024 13:21
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.

5 participants