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: Added login and register capability in discovery for pre login exploration #202

Conversation

saeedbashir
Copy link
Contributor

This PR adds the functionality of sign-in and register in the discovery and discovery course detail page when the discovery accessed from the startup screen (pre-login exploration). The pre-login experience is being controlled with the feature flag PRE_LOGIN_EXPERIENCE_ENABLED

Here is the screen recording of the feature:

Screen.Recording.2023-12-13.at.11.26.59.AM.mov

@saeedbashir saeedbashir force-pushed the saeed/pre_login_exploration_discovery branch from 57529b6 to b7ab020 Compare December 13, 2023 07:10
@saeedbashir saeedbashir requested a review from rnr December 13, 2023 10:44
rnr
rnr previously approved these changes Dec 13, 2023
@rnr
Copy link
Contributor

rnr commented Dec 13, 2023

@saeedbashir please look on Tests - something was failed there. Thank you

@rnr
Copy link
Contributor

rnr commented Dec 13, 2023

@saeedbashir I think needs to add @volodymyr-chekyrta as Reviewer also. Thank you

@saeedbashir
Copy link
Contributor Author

@saeedbashir I think needs to add @volodymyr-chekyrta as Reviewer also. Thank you

I was waiting for an approval from our team. I have now requested a review from @volodymyr-chekyrta

rnr
rnr previously approved these changes Dec 14, 2023
Copy link
Contributor

@shafqat-muneer shafqat-muneer left a comment

Choose a reason for hiding this comment

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

@saeedbashir I encounter an error when I click on the View Course button on the course dashboard. Kindly investigate this issue.

Additionally, I have included some comments. Please review them as well.

@@ -83,7 +89,7 @@ public struct StartupView: View {
}
.padding(.horizontal, isHorizontal ? 10 : 24)

LogistrationBottomView(viewModel: viewModel)
LogistrationBottomView(viewModel: viewModel, sourceScreen: .startup)
Copy link
Contributor

Choose a reason for hiding this comment

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

In various locations, we utilize LogistrationSourceScreen.startup, while here it employs .startup. It is necessary to ensure uniformity.

Discovery/Discovery/Presentation/DiscoveryViewModel.swift Outdated Show resolved Hide resolved
@miankhalid miankhalid changed the title feat: Added login and register capebility in discovery for pre login exploration feat: Added login and register capability in discovery for pre login exploration Dec 18, 2023
@saeedbashir
Copy link
Contributor Author

@volodymyr-chekyrta It's ready for another pass.

@volodymyr-chekyrta
Copy link
Contributor

No other requests from my side; we can address current feedback and merge.
@saeedbashir Thank you!

@saeedbashir
Copy link
Contributor Author

saeedbashir commented Dec 19, 2023

No other requests from my side; we can address current feedback and merge.
@saeedbashir Thank you!

@volodymyr-chekyrta Merge is disabled for me; I guess it will get enabled after your approval.

@volodymyr-chekyrta
Copy link
Contributor

No other requests from my side; we can address current feedback and merge.
@saeedbashir Thank you!

@volodymyr-chekyrta Merge is disabled for me; I guess it will get enabled after your approval.

Here is some miscommunication.
Could you please take a look at my comments above?
I can approve and merge it right after the fixes.
Thank you!

@saeedbashir
Copy link
Contributor Author

I can approve and merge it right after the fixes.

Oh my bad, I'd already made those changes but forgot to push them. Just pushed.

@volodymyr-chekyrta
Copy link
Contributor

I can approve and merge it right after the fixes.

Oh my bad, I'd already made those changes but forgot to push them. Just pushed.

Thank you! I resolved threads about code formatting.

Could you please check the threads about the DiscoveryView
https://github.com/openedx/openedx-app-ios/pull/202/files/9289ff22273731158c1b5aba28423eaa1533789d#diff-826c415701591643c9dc301590ff89a1631a73b6c3214ed1f6961466e72749a7

We have to decouple modules 🙏; we can merge after that.

Thank you!

@saeedbashir
Copy link
Contributor Author

I can approve and merge it right after the fixes.

Oh my bad, I'd already made those changes but forgot to push them. Just pushed.

Thank you! I resolved threads about code formatting.

Could you please check the threads about the DiscoveryView https://github.com/openedx/openedx-app-ios/pull/202/files/9289ff22273731158c1b5aba28423eaa1533789d#diff-826c415701591643c9dc301590ff89a1631a73b6c3214ed1f6961466e72749a7

We have to decouple modules 🙏; we can merge after that.

Thank you!

@volodymyr-chekyrta Done

@saeedbashir
Copy link
Contributor Author

@volodymyr-chekyrta I guess every comment is addressed now. I'm not sure why, but somehow the complete review was missed.

@volodymyr-chekyrta
Copy link
Contributor

@saeedbashir Thank you!
Let's wait for the build and tests to be sure that nothing is broken and merge it 🚀🚀🚀

@volodymyr-chekyrta volodymyr-chekyrta merged commit 65b1ecc into openedx:develop Dec 19, 2023
3 checks passed
saeedbashir added a commit to saeedbashir/openedx-app-ios that referenced this pull request Jan 5, 2024
…exploration (openedx#202)

* feat: added login and register capebility in discovery for pre login exploration

* refactor: address review feedback

* fix: fix broken tests

* fix: linking authorization framework with discovery framework

* refactor: address review feedback

* refactor: address review feedback

* fix: fix broken tests after changes

* refactor: address review feedback

* refactor: changing LogistrationBottomView style from model to closure and moving it to Core

* refactor: address review feedback

* refactor: decouple Authorization and Discovery module, remove unused imports

* refactor: removing unused imports
saeedbashir added a commit to saeedbashir/openedx-app-ios that referenced this pull request Jan 5, 2024
…exploration (openedx#202)

* feat: added login and register capebility in discovery for pre login exploration

* refactor: address review feedback

* fix: fix broken tests

* fix: linking authorization framework with discovery framework

* refactor: address review feedback

* refactor: address review feedback

* fix: fix broken tests after changes

* refactor: address review feedback

* refactor: changing LogistrationBottomView style from model to closure and moving it to Core

* refactor: address review feedback

* refactor: decouple Authorization and Discovery module, remove unused imports

* refactor: removing unused imports
@saeedbashir saeedbashir deleted the saeed/pre_login_exploration_discovery branch January 17, 2024 06: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