Closed IvanStepanok closed 3 weeks 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.
Hi @IvanStepanok, thanks for these changes. They look really good overall. I've noted all the minor visual issues I could find below. This is a huge usability/experience win regardless of these issues, so if any are difficult to resolve, most of them could be saved for a future cleanup.
iOS:
Android:
Both iOS and Android:
Hi @sdaitzman, and thanks for paying attention to the details! I appreciate it🙌
✅ 1. In light and dark mode, the Resume course quick action doesn’t need a border separating it from the other actions. ✅ 2. In dark mode, I think the View All Courses card may be a bit too dark/difficult to distinguish from the background. ✅ (2) The account settings gear in the top right should switch to the updated icon
âś… 3. In the All Courses view in light mode, the filter options currently use a light gray border color, and keep that border color when selected. âś… 4. Similarly, in dark mode, course filter options should have no border and should just change their background from Half Dark to Accent when selected.
âś… 5. When the Programs/Courses dropdown switcher is tapped, the dropdown icon should animate
https://github.com/openedx/openedx-app-ios/assets/128456094/a09acf1b-5a18-4653-b65d-71d6362839ce
⛔️(3) Selected dashboard-level tabs in dark mode should switch over to using the Dark Theme Accent Text color Changing the AccentColor can affect other parts of the application. I think it will be better to make these changes in another round. ⛔️ (4) Currently, the deep-link from some quick actions like upcoming assignments/resume course jump to the course home. In these cases, we need to preload the course structure to move forward. We can't skip this step:(
Hey @IvanStepanok, thanks so much for these updates! This looks all good to me now, and we can hold (3) and (4) for future improvements
referring to point 5 here.
@sdaitzman shouldn't the Course/Program switcher be animated in the reverse order? It should be pointing down by default which indicates a dropdown and should be pointing up once opened?
@moiz994 great catch, thank you! Yes, the Course/Program switcher dropdown icon should be flipped.
CC @IvanStepanok
@sdaitzman @moiz994 Done 🙌
https://github.com/openedx/openedx-app-ios/assets/128456094/c670aec8-a6e3-48fb-8ccd-4954fe3555d4
@IvanStepanok Maybe we want to scroll through the menu a bit when opening via the "View Assignments" button to see the currently selected item? @sdaitzman @moiz994 wdyt?
@sdaitzman @moiz994 @IvanStepanok Just a question - should these options be available for offline (if we know it won't load offline anyway)?
https://github.com/openedx/openedx-app-ios/assets/37253/f93e4f22-4b44-4916-8c8e-39074b2b2afe
@rnr thanks for checking about these options.
For the first question: yes, I think it makes sense to scroll the course header navigation to the active tab when jumping there.
For the second question, I think it depends on the plan for implementing offline course storage improvements. If those views are accessible offline in the future, the shortcuts to them should be as well.
I would lean slightly towards keeping the quick actions for now and showing a message like the "Slow or no internet connection" message that appears on the bottom of course home in that video until the content is available offline in the future. I'm curious what others think about this question though— can bring it up next mobile design weekly call if that would be helpful.
@rnr Thanks for the review! All the suggested changes have been addressed and corrected.🔥
Just to report: I've added a new issue with the double refresh of the Dates tab (it's not related to this PR).
@saeedbashir Thanks for review!
I have implemented this change as requested. It was a valid point.
This issue has been fixed.
Currently, we are using the old search screen. The logic for this feature will be defined in a future scope.
@rnr has created a new issue for this bug, as mentioned earlier.
Hi @IvanStepanok, on the third question, I think it would make sense to hide the search feature entirely on the View All Courses screen over reusing the all-catalog search there. A user initiating a search from that page is very likely intending to search only enrolled courses. Since that's out of scope for current development, I think it makes more sense to hide the feature entirely rather than search the full catalog.
The vast majority of users will be able to find all their courses without using search, especially by using the filters.
@IvanStepanok @volodymyr-chekyrta Is it documented that how a course will be moved to the primary course? I'm trying to figure it out but I'm not getting the logic. I tried by going through a course that isn't a primary course, and I've completed multiple components of that course, but still, the course wasn't moved to the primary course.
Sometimes the same approach works, and sometimes it doesn't.
@saeedbashir 🙌
As Sam suggested, please remove search from View All Course page as it's misleading / not completed. âś… I have removed the search bar as requested.
I'm moving to the course home and facing an issue in API. Then there isn't a going back. We might create an issue if it doesn't fall under the scope of current PR. âś… Yes, we will need to address this in a separate scope.
It seems the touch is being ignored when I click on course to go to the course home. Ideally, the touch shouldn't be ignored. The touch will do the action and close the opened menu.
⚠️ Unfortunately, I haven't been able to come up with a better solution for this issue. If you have any suggestions, they would be greatly appreciated.
Wouldn't this be an overhead by refreshing enrollments on each component completed? âś… Thank you for pointing that out. I have adjusted the logic so that enrollments are now refreshed only once.
@saeedbashir About this: It seems the touch is being ignored when I click on course to go to the course home. Ideally, the touch shouldn't be ignored. The touch will do the action and close the opened menu.
I conducted a little investigation, and from what I observed in various software, this behavior is standard. The first click closes the open dropdown menu, and the second click performs the intended action.
I conducted a little investigation, and from what I observed in various software, this behavior is standard. The first click closes the open dropdown menu, and the second click performs the intended action.
@IvanStepanok I'm not sure about the other apps, but this is how it is working in the production app.
https://github.com/openedx/openedx-app-ios/assets/5606473/83298c25-36fd-401b-9212-208f1ab425e2
@sdaitzman Could you please take a look and let us know wether the touch ignore is acceptable behaviour. The video is attached in the review feedback. https://github.com/openedx/openedx-app-ios/pull/434#pullrequestreview-2068121270
@sdaitzman
I investigated the behavior of dropdown menus in various applications. Consistently, the first click outside an open dropdown closes it, and the second click performs the intended action.
I've attached a video showing this behavior in multiple production apps:
https://github.com/openedx/openedx-app-ios/assets/128456094/f6e017a5-b63d-4674-ae45-8cfbc1bd555e
https://github.com/openedx/openedx-app-ios/assets/128456094/cd7f708c-c7d1-4f74-ab49-8c66f1259895
https://github.com/openedx/openedx-app-ios/assets/128456094/a7e3bc05-821d-48a6-9cdb-b3f5e1fe514c
https://github.com/openedx/openedx-app-ios/assets/128456094/b37fd344-3a3e-471d-8b24-7514d706402b
https://github.com/openedx/openedx-app-ios/assets/128456094/aa843dc2-a7f7-45a9-bce8-0cc680ed0391
This behavior is expected for several reasons:
User Expectation: Users expect that clicking outside a dropdown will close it. Preventing Accidental Actions: This prevents unintended actions from occurring. Consistency: It aligns with user experience across different applications. This standard behavior enhances usability by providing a predictable interface. Please review the video and consider these points.
Hey @IvanStepanok @saeedbashir— I agree that the expected behavior when tapping outside an open menu is to close the menu, not to perform another action
@IvanStepanok @volodymyr-chekyrta Is it documented that how a course will be moved to the primary course? I'm trying to figure it out but I'm not getting the logic. I tried by going through a course that isn't a primary course, and I've completed multiple components of that course, but still, the course wasn't moved to the primary course.
Sometimes the same approach works, and sometimes it doesn't.
@sdaitzman Is it possible for you to share details of the story https://youtrack.raccoongang.com/issue/AXM-506. I'm going through the content of a non-primary course and the completion API has been calling for multiple components, but still, I'm getting the old primary course in the enrollments.
cc @KyryloKireiev
@IvanStepanok In the list dashboard view, the Programs tab has disappeared. Please make sure it will also be configured the old way (in the bottom tab bar).
@IvanStepanok In the list dashboard view, the Programs tab has disappeared. Please make sure it will also be configured the old way (in the bottom tab bar).
Thank you for bringing this to my attention. I have addressed the issue, and the Programs tab has been restored to the bottom tab bar as before.
@IvanStepanok @volodymyr-chekyrta Is it documented that how a course will be moved to the primary course? I'm trying to figure it out but I'm not getting the logic. I tried by going through a course that isn't a primary course, and I've completed multiple components of that course, but still, the course wasn't moved to the primary course. Sometimes the same approach works, and sometimes it doesn't.
@sdaitzman Is it possible for you to share details of the story https://youtrack.raccoongang.com/issue/AXM-506. I'm going through the content of a non-primary course and the completion API has been calling for multiple components, but still, I'm getting the old primary course in the enrollments.
cc @KyryloKireiev
@saeedbashir Thank you for your review!
Let’s move forward with this PR so we can continue developing the related features.
It’s indeed odd that you don’t see a new primary course after xBlock completion; this might be a sandbox issue. However, this is not within the iOS area. All back-end issues will be addressed in the back-end PR, and we will cover any minor issues by the next PRs.
For now PR looks good for merging to me.
@GlugovGrGlib FYI sandbox issue.
cc @marcotuts @e0d @miankhalid
Let’s move forward with this PR so we can continue developing the related features.
@volodymyr-chekyrta The PR isn't block because of this issue/requirements, I just needed documentation of this so that QA can update the test document.
Let’s move forward with this PR so we can continue developing the related features.
@volodymyr-chekyrta The PR isn't block because of this issue/requirements, I just needed documentation of this so that QA can update the test document.
I'll provide it asap
@IvanStepanok I'm not sure why but the program webview is not loading with course dashboard view. I've used the v3 of enrollments so the enrollments screen is not loading (any how this can happen anytime).
I know programs are not available on your intances, what you can do is you can use any dummy URL for programs, e.g www.courses.edx.org
and make sure it will display under the programs menu. Even the empty course screen is not hidden when switching to the program view.
Another issue is, when I use pull to refresh the programs view, it calls the enrollments API instead of reloading the programs webview. The program's webview is working fine in list view.
https://github.com/openedx/openedx-app-ios/assets/5606473/885bb1d6-6b1d-4fb2-9273-bba1dd52cae4
@IvanStepanok @volodymyr-chekyrta Is it documented that how a course will be moved to the primary course? I'm trying to figure it out but I'm not getting the logic. I tried by going through a course that isn't a primary course, and I've completed multiple components of that course, but still, the course wasn't moved to the primary course. Sometimes the same approach works, and sometimes it doesn't.
@sdaitzman Is it possible for you to share details of the story https://youtrack.raccoongang.com/issue/AXM-506. I'm going through the content of a non-primary course and the completion API has been calling for multiple components, but still, I'm getting the old primary course in the enrollments.
cc @KyryloKireiev
Hi @saeedbashir, unfortunately I can't access that youtrack link. I think the intended design is that the most recent course where a user has completed blocks is highlighted in the primary course card on their dashboard.
Let’s move forward with this PR so we can continue developing the related features.
@volodymyr-chekyrta The PR isn't block because of this issue/requirements, I just needed documentation of this so that QA can update the test document.
@saeedbashir, please find the feature documentation below
Dashboard Level Navigation: Primary course This feature allows the mobile application to display user course enrollments, prioritizing the most relevant course information. The application shows a primary course card, secondary course cards, and provides an option to view all courses. This feature ensures users can easily access and manage their current course progress.
User Story As a user of the mobile application, I want to see my most relevant course enrollment details prioritized, so that I can quickly access and manage my current course progress efficiently.
Acceptance Criteria:
7. Offline Access: • I should be able to access the screen with the primary course information even when I am offline, ensuring continuity in tracking my course progress.
@volodymyr-chekyrta Is offline support also part of this PR. I'm asking this because it's not working.
- Offline Access: • I should be able to access the screen with the primary course information even when I am offline, ensuring continuity in tracking my course progress.
@volodymyr-chekyrta Is offline support also part of this PR. I'm asking this because it's not working.
@saeedbashir, Yes, according to the documentation, the primary course screen should be cached.
@IvanStepanok could you double-check what happened with caching?
@saeedbashir, @volodymyr-chekyrta,
I've made changes to handle the offline access issue by ensuring that the CoreData model is updated correctly. The main changes include:
Clearing Old Data: Before saving new enrollments, all existing data in the CoreData model is deleted. This approach avoids potential migration issues and ensures data consistency. Saving New Data: The new data is then saved into CoreData. These changes should help in caching the primary course screen correctly, allowing access to the course information even when offline.
It's worth considering applying a similar approach to other parts of the project that interact with CoreData to ensure overall data consistency and integrity. We should review the rest of the project to identify and address any similar issues.
@IvanStepanok Great! the offline mode is now working. Now the last item remaining in this PR is https://github.com/openedx/openedx-app-ios/pull/434#issuecomment-2130095030 after that we will be good to merge this PR.
@IvanStepanok I'm not sure why but the program webview is not loading with course dashboard view. I've used the v3 of enrollments so the enrollments screen is not loading (any how this can happen anytime).
I know programs are not available on your intances, what you can do is you can use any dummy URL for programs, e.g
www.courses.edx.org
and make sure it will display under the programs menu. Even the empty course screen is not hidden when switching to the program view.Another issue is, when I use pull to refresh the programs view, it calls the enrollments API instead of reloading the programs webview. The program's webview is working fine in list view.
Screen.Recording.2024-05-24.at.10.38.29.PM.mov
As an example, I try to add a base URL for Programs. They run the redirect flow, but if we comment out the block about redirecting, the programs view loads fine. Please check it on your side.🙌
@IvanStepanok I tried to capture the behaviour in the following screen recording for both case (new user with no enrollments and a user with enrollments). Please have a look, and if something isn't clear, we can connect on Slack.
https://github.com/openedx/openedx-app-ios/assets/5606473/b5bf585d-e629-413b-b243-6e597cff61d2
@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.
Quick Preview
https://github.com/openedx/openedx-app-ios/assets/128456094/4993cb6d-a9bb-4275-84bc-e3c75f1913a0
Drop down menu
They are enabled automatically when "Programs" is enabled in the configuration file.
https://github.com/openedx/openedx-app-ios/assets/128456094/6d17a0c3-a6fa-4a14-b204-e3994c88bb1b
Empty State
Horizontal Position
https://github.com/openedx/openedx-app-ios/assets/128456094/9d6e4376-62a1-453f-abba-2b61f04738c7
Feature enabled by default.
🚨 You should use key:
to enable a list style Dashboard (old style)
API
Since the new APIs are not available in the master branch, please use the sandbox: