Closed IvanStepanok closed 1 month ago
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:
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.
@IvanStepanok, please check this, the profile data in ProfileView does not change if I change it on the manage account screen.
https://github.com/openedx/openedx-app-ios/assets/127732735/d33b2499-5664-4dc9-a07c-d9e78829efb2
Please add navigation titles for each controller and fix long tap back navigation:
https://github.com/openedx/openedx-app-ios/assets/480059/7c057280-dd18-4c0d-9450-e7185ff40261
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 |
---|---|---|
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!🙏
https://github.com/openedx/openedx-app-ios/assets/128456094/1c58c108-68f7-4c21-9385-b3846fa159fc
Started reviewing PR
The location of the back button varies across screens in landscape mode.
Screens |
---|
Thank you, @shafqat-muneer, for the review! All requests have been satisfied. Ready for another pass 🙌
@IvanStepanok Thank you for considering the feedback. 🎉 Overall, I have a few additional minor observations that might require attention.
Settings | Manage Account |
---|---|
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.
https://github.com/openedx/openedx-app-ios/assets/11990137/c138dbdd-5c6a-4f7a-8f8c-4909858a058f
Hi, @shafqat-muneer, and thanks for the review! All problems are fixed. 🙌
@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.
Hi, @shafqat-muneer, please take a look 🙌
Hi @IvanStepanok Thanks LGTM 👍
@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.
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.![ezgif-1-d38260a5e3](https://github.com/openedx/openedx-app-ios/assets/128456094/9d04572d-2a23-4a9f-bec7-bbeb567b5868)
Light and Dark themes:
Settings screen:
Manage account:
Landscape:
Ipad: