feat: [FC-0047] Account Settings and Profile split #397

IvanStepanok · 2024-04-17T19:14:19Z

Split account settings and the user profile

Design: https://www.figma.com/file/iZ56YMjbRMShCCDxqrqRrR/Open-edX-Mobile-App-All-Screens-v2.1?type=design&node-id=0-1&mode=design

Quick demo:

Setting button on Discover, Dashboard and Profile screens.

Light and Dark themes:

Settings screen:

Manage account:

Landscape:

Ipad:

openedx-webhooks · 2024-04-17T19:14:23Z

Thanks for the pull request, @IvanStepanok! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

volodymyr-chekyrta · 2024-04-22T07:53:52Z

default_config/dev/config.yaml

-
+

Seems that a undesired change, revert it.

volodymyr-chekyrta · 2024-04-22T07:55:02Z

Authorization/Authorization/Presentation/Login/SignInView.swift

@@ -27,7 +27,7 @@ public struct SignInView: View {
    public var body: some View {
        ZStack(alignment: .top) {
            VStack {
-                ThemeAssets.authBackground.swiftUIImage
+                ThemeAssets.titleBackground.swiftUIImage


How about renaming it to headerBackground?

volodymyr-chekyrta · 2024-04-22T08:00:45Z

Core/Core/SwiftGen/Assets.swift

@@ -111,6 +112,7 @@ public enum CoreAssets {
  public static let noWifiMini = ImageAsset(name: "noWifiMini")
  public static let notAvaliable = ImageAsset(name: "notAvaliable")
  public static let playVideo = ImageAsset(name: "playVideo")
+  public static let settingsIcon = ImageAsset(name: "settingsIcon")


Since we don't use the word icon for icons, how about just renaming it to settings?

volodymyr-chekyrta · 2024-04-22T08:06:50Z

@IvanStepanok, please check this, the profile data in ProfileView does not change if I change it on the manage account screen.

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-04-22.at.11.03.24.mp4

volodymyr-chekyrta · 2024-04-22T08:08:23Z

Profile/Profile/Presentation/Profile/ProfileView.swift

-                    .foregroundColor(Theme.Colors.textSecondary)
+        if viewModel.userModel?.shortBiography != "" {
+            VStack(alignment: .leading, spacing: 6) {
+                Text("About Me") // Add Localization, delete old ProfileLocalization.info


Move to resources

volodymyr-chekyrta · 2024-04-22T08:11:30Z

Profile/Profile/Presentation/Settings/SettingsView.swift

+                            Button(action: { viewModel.router.back() }, label: {
+                                CoreAssets.arrowLeft.swiftUIImage.renderingMode(.template)
+                                    .backButtonStyle(color: Theme.Colors.loginNavigationText)
+                            })


Please format to the each param on own line.

volodymyr-chekyrta · 2024-04-22T08:12:09Z

Profile/Profile/Presentation/Settings/SettingsView.swift

+    // MARK: - Manage Account
+    @ViewBuilder
+    private var manageAccount: some View {
+


Please remove extra space

volodymyr-chekyrta · 2024-04-22T08:12:35Z

Profile/Profile/Presentation/Settings/SettingsView.swift

+                }
+            })
+            .accessibilityIdentifier("video_settings_button")
+


Please remove extra space

volodymyr-chekyrta · 2024-04-22T08:13:14Z

Profile/Profile/Presentation/Settings/SettingsView.swift

+        .cardStyle(bgColor: Theme.Colors.textInputUnfocusedBackground,
+                   strokeColor: .clear)


Please format each parameter on its own line.

volodymyr-chekyrta · 2024-04-22T08:13:51Z

Profile/Profile/Presentation/Settings/SettingsView.swift

+                            Task {
+                                await viewModel.logOut()
+                            }
+                        }, type: .logOut


Please format each parameter on its own line.

volodymyr-chekyrta · 2024-04-22T09:48:30Z

Profile/Profile/Presentation/Settings/ManageAccountView.swift

+                            Button(action: { viewModel.router.back() }, label: {
+                                CoreAssets.arrowLeft.swiftUIImage.renderingMode(.template)
+                                    .backButtonStyle(color: Theme.Colors.loginNavigationText)
+                            })


Please format each parameter on its own line.

forgotvas · 2024-04-22T10:51:06Z

Please add navigation titles for each controller and fix long tap back navigation:

2024-04-22.13.49.11.mov

forgotvas · 2024-04-22T11:56:16Z

Is that design decision to hide info on profile overview? In develop build i can see "year of birth" and "bio", in that branch only bio and only on profile screen:

develop	profile	Manage account

fix navigation titles and back buttons on few screens

IvanStepanok · 2024-04-25T08:34:15Z

Hi @forgotvas and thanks for the review!
The year of birth was removed by design, it's true, I double-checked it:
https://www.figma.com/file/iZ56YMjbRMShCCDxqrqRrR/Open-edX-Mobile-App-All-Screens-v2.1?type=design&node-id=0-1&mode=design

I fixed the back buttons and titles in the video settings too. Thanks!🙏

Screen.Recording.2024-04-25.at.11.29.01.mov

forgotvas

LGTM 👍

shafqat-muneer · 2024-04-26T10:28:49Z

Started reviewing PR

shafqat-muneer · 2024-04-26T11:39:21Z

The location of the back button varies across screens in landscape mode.

Screens

shafqat-muneer

The review is currently underway. Kindly attend to the suggested changes made up to this stage.

shafqat-muneer · 2024-04-26T11:45:07Z

Profile/Profile/Presentation/EditProfile/EditProfileView.swift

@@ -260,7 +251,8 @@ struct EditProfileView_Previews: PreviewProvider {
            yearOfBirth: 0,
            country: "Ukraine",
            shortBiography: "",
-            isFullProfile: true
+            isFullProfile: true, 


Please remove trailing whitespace to address warning:

Trailing Whitespace Violation: Lines should not have trailing whitespace (trailing_whitespace)

shafqat-muneer · 2024-04-26T14:26:20Z

...se/Presentation/Subviews/VideoDownloadQualityBarView/VideoDownloadQualityContainerView.swift

    }

    var body: some View {
        NavigationView {
            VideoDownloadQualityView(
                downloadQuality: downloadQuality,
                didSelect: didSelect,
-                analytics: analytics
+                analytics: analytics, 


Trailing Whitespace Violation

shafqat-muneer · 2024-04-26T14:37:11Z

OpenEdX/Router.swift

@@ -694,6 +694,20 @@ public class Router: AuthorizationRouter,
        navigationController.pushViewController(controller, animated: true)
    }

+    public func showVideoSettings() {
+        let viewModel = Container.shared.resolve(SettingsViewModel.self)!


Let's avoid force unwrapping to avoid potential crash. We can update like this:

guard let viewModel = Container.shared.resolve(SettingsViewModel.self) else { return }

Thank you for your suggestion to use a guard-let to avoid potential crashes by force unwrapping. While this is generally good practice, in this specific context, the force unwrap is intentional. Our application's design guarantees that the SettingsViewModel will always be present in the container when this function is called. Introducing a guard-let would imply a possibility of SettingsViewModel not being available, which contradicts our current architecture and could potentially confuse future maintainers about the assumptions in our codebase. Let's keep the force unwrap as an assertion of our design guarantees.

shafqat-muneer · 2024-04-26T14:37:24Z

OpenEdX/Router.swift

+    }
+
+    public func showManageAccount() {
+        let viewModel = Container.shared.resolve(ManageAccountViewModel.self)!


guard let viewModel = Container.shared.resolve(ManageAccountViewModel.self) else { return }

shafqat-muneer · 2024-04-26T14:42:36Z

OpenEdX/View/MainScreenView.swift

@@ -17,7 +17,6 @@ import Theme

 struct MainScreenView: View {

-    @State private var settingsTapped: Bool = false
    @State private var disableAllTabs: Bool = false
    @State private var updateAvaliable: Bool = false


typo: updateAvailable

shafqat-muneer · 2024-04-26T14:53:58Z

Profile/Profile/Presentation/Profile/ProfileView.swift

-
+

Please remove new line.

shafqat-muneer · 2024-04-26T14:54:52Z

Profile/Profile/Presentation/Profile/ProfileView.swift

-                    .foregroundColor(Theme.Colors.textSecondary)
+        if viewModel.userModel?.shortBiography != "" {
+            VStack(alignment: .leading, spacing: 6) {
+                Text(ProfileLocalization.about) 


Trailing Whitespace Violation

shafqat-muneer · 2024-04-26T15:02:18Z

Profile/Profile/Presentation/Profile/ProfileView.swift

-                    .padding(.horizontal, 24)
-                    .font(Theme.Fonts.labelLarge)
-                    .foregroundColor(Theme.Colors.textSecondary)
+        if viewModel.userModel?.shortBiography != "" {


It seems, we can update like:

if let bio = viewModel.userModel?.shortBiography, bio != "" { VStack(alignment: .leading, spacing: 6) { Text(ProfileLocalization.about) .font(Theme.Fonts.titleSmall) .foregroundColor(Theme.Colors.textPrimary) .accessibilityIdentifier("profile_info_text") Text(bio) .font(Theme.Fonts.bodyMedium) .foregroundColor(Theme.Colors.textPrimary) .accessibilityIdentifier("bio_text") }

shafqat-muneer · 2024-04-26T15:04:48Z

Profile/Profile/Presentation/Profile/ProfileView.swift

-
+    
    private var url: URL?
    @Binding private var image: UIImage?
-
+    


Please discard this change.

shafqat-muneer · 2024-04-26T15:04:55Z

Profile/Profile/Presentation/Profile/ProfileView.swift

-
+

Please discard this change.

IvanStepanok · 2024-04-30T12:14:58Z

Thank you, @shafqat-muneer, for the review! All requests have been satisfied. Ready for another pass 🙌

shafqat-muneer · 2024-04-29T16:11:04Z

Profile/Profile/Presentation/Settings/SettingsView.swift

+                                ProgressBar(size: 40, lineWidth: 8)
+                                    .padding(.top, 200)
+                                    .padding(.horizontal)
+                                    .accessibilityIdentifier("progressbar")


Based on the convention being followed, shouldn't it be named progress_bar?

shafqat-muneer · 2024-04-30T14:18:47Z

Profile/Profile/Presentation/Settings/VideoQualityView.swift

+                                    .padding(.horizontal)
+                                    .accessibilityIdentifier("progressbar")
+                            } else {
+


Please remove new line

shafqat-muneer · 2024-04-30T14:38:50Z

Profile/Profile/en.lproj/Localizable.strings

+"ABOUT" = "About Me";
+"EDIT_PROFILE" = "Edit Profile";


Is the use of all caps intentional, given that conventionally, all words except the first one are in small caps? If it's according to design then we can communicate to designer about the protocol which is being followed across the application.

If it's addressable then need to address to all other places.

shafqat-muneer · 2024-04-30T14:55:12Z

Profile/ProfileTests/Presentation/Settings/SettingsViewModelTests.swift

+        )
+
+
+        await viewModel.logOut()
+


Please remove extra newline.

shafqat-muneer · 2024-05-01T08:21:44Z

@IvanStepanok Thank you for considering the feedback. 🎉
Overall, I have a few additional minor observations that might require attention.

Observed overlapping on manage account and settings screens

Settings	Manage Account

Safe area is visible in Video download quality screen and it's covered in Video settings screen.

Video download quality	Video settings

The position of the back arrow appears to be incorrect in comparison to the other screens.
Finally, although it may not be directly related to the changes made in this pull request, I noticed it while reviewing the PR. We might want to address it in a separate PR. In the pre-login user experience, the course details aren't opening; it consistently shows an error.

Simulator.Screen.Recording.-.iPhone.15.-.2024-05-01.at.13.19.39.mp4

IvanStepanok · 2024-05-04T11:08:40Z

Hi, @shafqat-muneer, and thanks for the review! All problems are fixed. 🙌

shafqat-muneer · 2024-05-06T06:26:14Z

@IvanStepanok Thanks for addressing the feedback. 🎉
All changes LGTM 👍. PR is approved.

It seems minor indentation / formatting related comments are missed. Please also address them before merge.

Separate issue is create here for this observed problem.

Finally, although it may not be directly related to the changes made in this pull request, I noticed it while reviewing the PR. We might want to address it in a separate PR. In the pre-login user experience, the course details aren't opening; it consistently shows an error.

IvanStepanok · 2024-05-07T10:00:49Z

Hi, @shafqat-muneer, please take a look 🙌

shafqat-muneer · 2024-05-07T12:03:23Z

Hi @IvanStepanok
Thanks
LGTM 👍

openedx-webhooks · 2024-05-07T13:14:12Z

@IvanStepanok 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

IvanStepanok added 4 commits April 11, 2024 11:01

feat: Separate account settings and profile editing

c3e6482

Merge branch 'develop' into feat/account_settings_and_profile_split

7ba797e

feat: changes according to desing

d6a5af7

feat: update background colors

57f8d71

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Apr 17, 2024

feat: update missed email value in tests

6d8d7ec

volodymyr-chekyrta changed the title ~~Feat/account settings and profile split~~ feat: [FC-0047] Account Settings and Profile split Apr 18, 2024

fix: resume old design

a745c7e

volodymyr-chekyrta self-requested a review April 22, 2024 07:49

volodymyr-chekyrta reviewed Apr 22, 2024

View reviewed changes

default_config/dev/config.yaml Outdated

Copy link

Contributor

volodymyr-chekyrta Apr 22, 2024

Choose a reason for hiding this comment

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

Seems that a undesired change, revert it.

volodymyr-chekyrta reviewed Apr 22, 2024

View reviewed changes

fix: changes after the code review

1fba243

volodymyr-chekyrta reviewed Apr 22, 2024

View reviewed changes

fix: code formatting on ManageAccountView

0805803

volodymyr-chekyrta marked this pull request as ready for review April 22, 2024 09:53

volodymyr-chekyrta requested review from rnr, forgotvas, saeedbashir and shafqat-muneer April 22, 2024 09:53

forgotvas removed the request for review from rnr April 22, 2024 10:57

IvanStepanok added 2 commits April 25, 2024 10:58

Merge branch 'develop' into feat/account_settings_and_profile_split

b24cc83

fix: changes after code review

c292ee8

fix navigation titles and back buttons on few screens

forgotvas previously approved these changes Apr 25, 2024

View reviewed changes

shafqat-muneer requested changes Apr 26, 2024

View reviewed changes

feat: updates after shafqat-muneer code review

8069fc7

IvanStepanok dismissed forgotvas’s stale review via 8069fc7 April 30, 2024 10:12

IvanStepanok requested a review from shafqat-muneer April 30, 2024 12:15

shafqat-muneer requested changes Apr 30, 2024

View reviewed changes

feat: fixes after review

d39c6f6

IvanStepanok requested a review from shafqat-muneer May 4, 2024 11:08

shafqat-muneer previously approved these changes May 6, 2024

View reviewed changes

feat: changes after code review

3239008

IvanStepanok dismissed shafqat-muneer’s stale review via 3239008 May 7, 2024 09:59

shafqat-muneer approved these changes May 7, 2024

View reviewed changes

volodymyr-chekyrta merged commit 4706755 into openedx:develop May 7, 2024
3 checks passed

volodymyr-chekyrta deleted the feat/account_settings_and_profile_split branch May 7, 2024 13:14

IvanStepanok mentioned this pull request Nov 6, 2024

Develop to main. Release v2 #538

Merged

		.cardStyle(bgColor: Theme.Colors.textInputUnfocusedBackground,
		strokeColor: .clear)

feat: [FC-0047] Account Settings and Profile split #397

feat: [FC-0047] Account Settings and Profile split #397

Conversation

IvanStepanok commented Apr 17, 2024 • edited Loading

Quick demo:

Light and Dark themes:

Settings screen:

Manage account:

Landscape:

Ipad:

openedx-webhooks commented Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

volodymyr-chekyrta commented Apr 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

volodymyr-chekyrta Apr 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

forgotvas commented Apr 22, 2024

forgotvas commented Apr 22, 2024 • edited Loading

IvanStepanok commented Apr 25, 2024

forgotvas left a comment

Choose a reason for hiding this comment

shafqat-muneer commented Apr 26, 2024

shafqat-muneer commented Apr 26, 2024

shafqat-muneer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IvanStepanok commented Apr 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shafqat-muneer commented May 1, 2024

IvanStepanok commented May 4, 2024

shafqat-muneer commented May 6, 2024

IvanStepanok commented May 7, 2024

shafqat-muneer commented May 7, 2024

openedx-webhooks commented May 7, 2024

IvanStepanok commented Apr 17, 2024 •

edited

Loading

openedx-webhooks commented Apr 17, 2024 •

edited

Loading

volodymyr-chekyrta Apr 22, 2024 •

edited

Loading

forgotvas commented Apr 22, 2024 •

edited

Loading