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

feat: use custom font and colors across the app where were missing #311

Merged
merged 23 commits into from
Mar 11, 2024

Conversation

saeedbashir
Copy link
Contributor

At a number of places, the default font was being used for so many elements, and the same was the case for colors. At a number of place, like the .red color was used at a number of places instead of the custom Alert color.

This PR fixes the issue and enhances the app theme capability by applying the app defined colors and app defined custom fonts across the app.

Note:

  1. This PR will not impact the openedX default theme. The colors scheme will remain the same.
  2. The default font is still being used for picker (discussion filters, discussion sort), as there isn't a straight-forward way to use custom fonts for picker items. Will look into it later.

rnr
rnr previously approved these changes Mar 1, 2024
rnr
rnr previously approved these changes Mar 6, 2024
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.

looks good to me
please fix one issue for edx dark mode
Screenshot 2024-03-06 at 13 39 26

@rnr
Copy link
Contributor

rnr commented Mar 6, 2024

I also see some font size changes for openEdX on the login page. Please confirm if this is acceptable.
Screenshot 2024-03-06 at 14 52 11

@saeedbashir
Copy link
Contributor Author

@rnr feedback addressed and I've corrected the size of sign-in screen elements

@saeedbashir saeedbashir requested a review from rnr March 6, 2024 15:31
@rnr
Copy link
Contributor

rnr commented Mar 7, 2024

looks good to me please fix one issue for edx dark mode Screenshot 2024-03-06 at 13 39 26

@saeedbashir please confirm we don't need border for Cancel button here in dark mode

rnr
rnr previously approved these changes Mar 7, 2024
@saeedbashir
Copy link
Contributor Author

saeedbashir commented Mar 8, 2024

looks good to me please fix one issue for edx dark mode Screenshot 2024-03-06 at 13 39 26

@saeedbashir please confirm we don't need border for Cancel button here in dark mode

The screenshot is not accessible. Anyhow, this is just a developer copy of the design so that the app looks presentable in dark mode. We might end up changing the whole color scheme after the design team reviews it.

Comment on lines 32 to 40
<key>UILaunchScreen</key>
<dict>
<key>UIColorName</key>
<string>SplashBackground</string>
<key>UIImageName</key>
<string>appLogo</string>
<key>UIImageRespectsSafeAreaInsets</key>
<true/>
</dict>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any chance not to replace the LaunchScreen.storyboard with the Info.plist configuration?
For some OeX instances we need a slightly more customizable LaunchScreen than just a background color and image, and this would be much easier with a storyboard file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there any chance not to replace the LaunchScreen.storyboard with the Info.plist configuration?

The background color of the storyboard isn't rendered at runtime, and it's selected/applied at selection time. And it's not a good way to parse and change the xml via white-label script to change the colors.

@rnr Is it possible to replace the whole LaunchScreen.storyboard through a white-label script, so providers can maintain their own LaunchScreen.storyboard?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the white-label script can just remove the Launch screen interface file base name key from the Info.plist and add UILaunchScreen?

Copy link
Contributor

Choose a reason for hiding this comment

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

Replacing the whole LaunchScreen.storyboard also an option 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

@volodymyr-chekyrta Yes, we can try both approaches. At first glance, replacing the LaunchScreen.storyboard file looks easier. @saeedbashir could you please prepare and add this file into edx-mobile-config repo and I will add whitelabel config and script changes?
Thank you

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@volodymyr-chekyrta It's re-added.

@saeedbashir saeedbashir merged commit e4bec6e into openedx:develop Mar 11, 2024
2 of 3 checks passed
@saeedbashir saeedbashir deleted the saeed/fonts branch March 11, 2024 13:47
@rnr rnr mentioned this pull request Mar 11, 2024
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] Using 'theme' font in all app's views
3 participants