Closed IvanStepanok closed 2 months 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.
Ready for product review
Issues with loading indicator: 1) Changed loading indicator for Course Home view (is it approved?):
Before | After |
---|---|
2) Loading indicator for "More" tab:
3) Different size on Dicussions View for Refresh indicator:
Issues with Scroll 1) When user scrolls on Discussion tab it doesn't collapse header (how it does on Home tab for example)
https://github.com/openedx/openedx-app-ios/assets/37253/3d66d306-396e-431c-9dbf-d4d07ea53166
2) Can't scroll to bottom on Dates tab (I guess it happens when content takes up a bit more space than is available on the screen). Strange animation for this case:
https://github.com/openedx/openedx-app-ios/assets/37253/74a1b81f-6d77-4226-bfdb-264da01fbcdf
The progress bar location is odd if we use pull to refresh while data is loading. and If we interact with UI white loading the bleeding header isn't working properly.
https://github.com/openedx/openedx-app-ios/assets/5606473/b33e3715-89c9-4198-8b45-c494d470905c
Old loading indicators are returned.
Fixed the βMoreβ loading indicator.
Pull to refresh indicators are equal now.
https://github.com/openedx/openedx-app-ios/assets/128456094/d34c4464-da9c-4d95-b08f-df2efec52c52
https://github.com/openedx/openedx-app-ios/assets/128456094/7666636d-e581-4fe7-81b5-15681eca1321
If the content height in ScrollView is not enough to collapse the header, collapsing mode are disabled. Thanks for paying attention to this bug, it's very important.π
https://github.com/openedx/openedx-app-ios/assets/128456094/b69909f2-198f-48da-8da0-7554da32484b
If you discover any new bugs, I'd be delighted to fix them!
If the content height in ScrollView is not enough to collapse the header, collapsing mode are disabled.
I think we need to disable collapse mode for the 'More' tab as it tries to collapse, but we have two rows (Distributions and Announcements) at the moment. Thank you
https://github.com/openedx/openedx-app-ios/assets/37253/a10624a8-7030-4582-8b95-349083877d4f
Is it a design decision to disable this collapse mode for iPad in general?
https://github.com/openedx/openedx-app-ios/assets/37253/b480a039-e9c7-417f-b9e1-a9514c94b27f
https://github.com/openedx/openedx-app-ios/assets/128456094/2ed59cae-2fde-4290-adf6-3edcfd47cc53
@rnr, @saeedbashir, thanks for the review, I appreciate it. Feel free to reach out if you find any new bugs or have questions. Thanks again!
Is it a design decision to disable this collapse mode for iPad in general?
Screen.Recording.2024-04-19.at.11.37.37.mov
Yes, it is approved with @sdaitzman
Header bar hides logo by half or more on iPad:
iPhone | iPad Portrait | iPad Landscape |
---|---|---|
UPDATE: It's expected behavior as teams decided on demo call.
Back button in wrong position for pages with header on iPad:
Course home | Course home (Landscape) | Content | Content (Landscape) |
---|---|---|---|
Controller with header doesn't have navigation title:
https://github.com/openedx/openedx-app-ios/assets/480059/26971cc7-bc31-4a92-b997-4b9c1c434bef
Resume button doesn't have paddings on top:
develop | FC-0047 |
---|---|
Pull to refresh indicator disappeared from videos tab:
Develop: https://github.com/openedx/openedx-app-ios/assets/480059/4ec46061-f710-4ca4-9e59-31c51a35a5a9
FC-0047: https://github.com/openedx/openedx-app-ios/assets/480059/21c9e8b9-091a-4179-beb1-022c0661eac6
Thanks @forgotvas, all your comments have been addressed! I've also vertically centered the banner so that it crops more evenly.
https://github.com/openedx/openedx-app-ios/assets/128456094/aabb0cf0-ca81-40ae-94af-e6cecfcd1dd3
Thanks, @IvanStepanok, just one was skipped - back button has wrong position it should be at same position as on content page:
course home | content |
---|---|
@forgotvas Thanks for the good idea! The position of the back button is consistent with the design, but I asked Sam Deitzman if it was possible to change the position and they confirmed that it would indeed be better. The Back button is now on the left.
https://github.com/openedx/openedx-app-ios/assets/128456094/e55ce86d-c80f-4010-af43-b66989934dae
@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.
This PR introduces updated navigation and banner for the course home.
Light and Dark theme:
![Simulator Screen Recording - iPhone 15 Pro Max - 2024-04-02 at 17 15 01](https://github.com/openedx/openedx-app-ios/assets/128456094/99166587-d409-4687-be6b-f593523ae5b1)
In the horizontal state, the top bar is always collapsed.![Simulator Screen Recording - iPhone 15 Pro Max - 2024-04-02 at 17 19 22](https://github.com/openedx/openedx-app-ios/assets/128456094/dc40b578-65b4-4141-9acc-47cae902f81f)
The iPad version doesn't have a collapsing effect.![Simulator Screen Recording - iPad (10th generation) - 2024-04-02 at 17 22 35](https://github.com/openedx/openedx-app-ios/assets/128456094/388d8c79-94f9-493a-b090-c154b7d74066)
:rotating_light: Breaking changes :rotating_light:: The
COURSE_BANNER_ENABLED
andCOURSE_TOP_TAB_BAR_ENABLED
feature flags have been removed.