Closed IvanStepanok closed 2 months ago
Thanks for the pull request, @IvanStepanok!
Please work through the following steps to get your changes ready for engineering review:
If you haven't already, check this list to see if your contribution needs to go through the product review process.
To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
This PR must be merged before / after / at the same time as ...
This PR is waiting for OEP-1234 to be accepted.
This PR must be merged by XX date because ...
This is for a course on edx.org.
If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.
This repository is currently maintained by @openedx/openedx-mobile-maintainers
. Tag them in a comment and let them know that your changes are ready for review.
If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:
Our goal is to get community contributions seen and reviewed as efficiently as possible.
However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
:bulb: As a result it may take up to several weeks or months to complete a review and merge your PR.
This looks great the videos are very helpful!
I did notice that after syncing it looks like Hide Inactive Courses was true / some courses were inactive but the setting toggle is off for this - Is that a toggle that will be update to work i na future PR?
Hi @marcotuts Sorry if it wasn't clear, I took a screen recording of this feature🙌
https://github.com/openedx/openedx-app-ios/assets/128456094/5039e04b-ce81-4316-b2fa-c82a1a33109a
Since this is a complex PR, lets wait for approvals from TouchApp and Axinite both before merging this PR, thanks!
I am unable to sync the calendar due to receiving a 404
response from the enrollments_status
API. It appears that this API has not been deployed in the production environment.
/api/mobile/v1/users/\(username)/enrollments_status/
It's blocker to review functionality.
https://github.com/openedx/openedx-app-ios/assets/11990137/dc6deade-1ede-432b-9c8f-3a0b71625496
Hi @shafqat-muneer, thank you so much for starting this review. The mentioned backend functionality was approved, but not yet merged into edx-platform master. Therefore, to not block review of the mobile implementation, please use the sandbox with all the latest features implemented for FC-0047, similarly to how it was done for Dashboard Level Navigation PR.
API
Since the new APIs are not available in the master branch, please use the sandbox:
API_HOST_URL: 'https://axim-mobile.raccoongang.net' OAUTH_CLIENT_ID: 'zP3vPz00c8fTRpYjNbVSlA1fxt9LnCxTM4JK1KQ0'
I hope this will unblock the further review process, thanks!
I am starting review today 🙌
When the 'Calendar Access' popup appears, tapping outside the alert area causes the screen overlay to disappear, but the alert itself remains on the screen. Ideally, the alert should also disappear when tapping anywhere outside of it.
https://github.com/openedx/openedx-app-ios/assets/11990137/8e7aec8a-b3ec-4f98-bf97-709b5349f2ed
I am unable to test the 'Hide Inactive Courses' toggle feature because there are no inactive courses available. I have enrolled in all available courses on the sandbox: https://axim-mobile.raccoongang.net
. Is there a way to test this feature? Do we have any test user with inactive courses available?
In the Figma designs, there is an option to use relative dates, but I do not see this feature in the current implementation.
If the calendar is unsynced, the following issues are observed when the user accesses the course:
To unsync the calendar, please follow these steps:
The review is currently ongoing, and I will continue with it tomorrow.
Hi, @shafqat-muneer. Happy to see you!
When the 'Calendar Access' popup appears, tapping outside the alert area causes the screen overlay to disappear, but the alert itself remains on the screen. Ideally, the alert should also disappear when tapping anywhere outside of it.
In the Figma designs, there is an option to use relative dates, but I do not see this feature in the current implementation
I am unable to test the 'Hide Inactive Courses' toggle feature because there are no inactive courses available. I have enrolled in all available courses on the sandbox: https://axim-mobile.raccoongang.net. Is there a way to test this feature? Do we have any test user with inactive courses available?
If the calendar is unsynced, the following issues are observed when the user accesses the course.
Additionally, key UX principles support this approach:
Predictability: Good interaction design is predictable, meaning users should know what will happen when they perform an action. If a user deletes an event, the expected outcome is its removal, fostering confidence and control (Give Good UX).
User Control: UX principles emphasize giving users control over their interactions. By allowing the event to stay deleted, we respect their autonomy and decision-making (UX Institute) (CareerFoundry).
Consistency and Logic: Ensuring the interface is logical and consistent helps users predict outcomes and feel more secure in their interactions. Ignoring a user’s deletion action could lead to confusion and frustration (Userpilot).
These principles ensure a positive user experience by making interactions clear, predictable, and user-controlled. Ignoring a user’s deliberate action undermines these core UX principles.
Our team thought about this behavior for a long time. In my opinion, based on UX/UI principles, the behavior must be predictable and we need to use the user control principle. So in case where user deletes an event, it can't be a mistake. The way to deleting an event takes a few screens. It is not about random swipe and deleting. It is a sequence of specific user actions. In this case, we can be sure, it is not a mistake. And if the user decides to delete some events, who we are to ignore their desires?
Thanks for all the design details. I completely agree with them. I created the scenario of an outdated calendar locally to demonstrate the use case. This situation can also occur if a teacher updates the course calendar from backend, potentially with important updates. The course calendar could become outdated without the user knowing and user can miss important updates. It will only sync when the user navigates to the Dates & Calendar
section. Ideally auto sync should be there in this case but if we decide against implementing auto-sync functionality upon landing on the course, we should at least provide some indication to the user that the course dates are not synced. Perhaps we can define state to show outdated calendar
course as we are showing synced, not synced, or offline states? 🤔
On dates tab, by tapping on calendar cell/section, user should redirect to Dates & Calendar screen. Redirection behavior is implemented on android side.
I removed calendar access from Settings and then navigated to the 'Dates & Calendar' section. Despite the console displaying NSLocalizedDescription=Access denied
, the application erroneously indicates that the calendar synced successfully.
https://github.com/openedx/openedx-app-ios/assets/11990137/5999ae30-8c59-4581-974f-94ecc2077301
Initially, unsynced courses appear under the Not Synced
tab. However, after marking them, they move to the Synced
tab and will be synced the next time the screen is loaded. Ideally, these courses should sync instantly. If that’s not possible, there should be an indication that these courses will be synced in the next iteration. For instance, we could add the text (in queue)
in front of each course name. The designer may have additional input, but there should be a clear distinction between courses that are synced and those that are queued for the next sync. Thoughts?
https://github.com/openedx/openedx-app-ios/assets/11990137/153ac00d-6b7e-48a1-967b-e8fa9ddb7beb
If no content is available, an empty screen message should ideally added.
It would be helpful to display a toast message to the user indicating the success or failure of the calendar sync. Currently, no toast messages are being shown.
On iPhone SE 3rd generation, sync status section hides under the tabs and header.
https://github.com/openedx/openedx-app-ios/assets/11990137/c76b576b-6a79-4640-ad8f-f72e70aa85f6
If the application is deleted and reinstalled, logging in with the same user results in duplicate events in the calendar. Is it possible to remove the duplicates or use the previously added events? Thoughts?
@shafqat-muneer Thanks for the review!
Perhaps we can define state to show outdated calendar course as we are showing synced, not synced, or offline states? 🤔
In MainScreenViewModel we have a updateCalendarIfNeeded() function for handling relevance of the calendar.
Issue 4: – Done ✅ Issue 5: – Done ✅ Issue 6: – Make sence. The word "Synced" sounds like a completed action. For a quick solution, I suggest renaming the button to "To Sync". In the long term perspective, we need to conduct research on user feedback and maybe ask the designer to make a redesign of this screen. But in my opinion, renaming the button to "To Sync" is enough. @sdaitzman any thoughts?🙌 Issue 7, 8: – @sdaitzman we need your deep expertise 🙏 Issue 9: – Already fixed by Vadim Kuznetsov ✅ Issue 10: – Great point💪 Fixed✅
Hi @shafqat-muneer, thank you for flagging these issues for design review
Issue 6 On the Synced/Not Synced tabs, when tapping a checkbox next to a course to begin syncing it, it’s important to show that sync progress is taking place, and then that it has completed. I think we can accomplish this by using platform-provided animated loaders to show syncing will be briefly taking place, then showing a checked box for several seconds, then animating the row out. Here’s a quick prototype flow of this microinteraction (Figma link):
Issue 7 Thanks for raising this! I added an empty state closely based on the course/course dashboard empty states (Figma link)
Issue 8 I think the updates under issue 6 above should resolve the usability issue without using toasts, but will follow up about this at the next mobile design weekly call for community input.
Let me know if this all makes sense, or if I can clarify/provide additional screens. Thanks!
Thank you @sdaitzman for the design feedback. Here are my further thoughts on it:
Issue 6
Based on the current implementation, when a user taps on any course from the 'Not Synced' tab, it immediately moves to the 'Synced' tab without initiating actual calendar synchronization. The synchronization for that course will only occur when the user revisits this screen at any time. If the user remains on this screen, the course will appear under the 'Synced' tab, but in reality, it has not been synchronized yet. We need to show any indication in Synced
tab. Because in reality, at this point, Synced
tab can have synced courses and flagged for sync courses, which will sync next revisit of the screen. I hope this further clarifies my point.
On the Synced/Not Synced tabs, when tapping a checkbox next to a course to begin syncing it, it’s important to show that sync progress is taking place, and then that it has completed. I think we can accomplish this by using platform-provided animated loaders to show syncing will be briefly taking place, then showing a checked box for several seconds, then animating the row out. Here’s a quick prototype flow of this microinteraction (Figma link):
Issue 7 LGTM 👍
Thanks for raising this! I added an empty state closely based on the course/course dashboard empty states (Figma link)
Issue 8 When the user arrives on the Dates & Calendar Sync screen and the courses calendar sync begins, I am referring to displaying a toast message after the sync process is complete (if it makes sense).
I think the updates under issue 6 above should resolve the usability issue without using toasts, but will follow up about this at the next mobile design weekly call for community input.
@shafqat-muneer, The UI changes have been applied, and all feedback has been addressed.
Regarding Issue 3, it appears to be a miscommunication. According to the new behavior, at every app start, we compare the date checksums from the server and the local version, replacing all outdated events. Meanwhile, we ignore all user interactions in the system calendar, considering them intentional.
I hope all these changes make you happier!🙏
I really appreciate @IvanStepanok for addressing the feedback. Due to the priority of our MVP release, I may not be able to verify the changes myself. @volodymyr-chekyrta, could you please verify the addressed changes so that this PR can be considered for merging?
I really appreciate @IvanStepanok for addressing the feedback. Due to the priority of our MVP release, I may not be able to verify the changes myself. @volodymyr-chekyrta, could you please verify the addressed changes so that this PR can be considered for merging?
@shafqat-muneer, sure I understand the situation with the release. I'll double-check everything, and @IvanStepanok will prepare a demo video with fixes. If we miss something, we will do a regression after the Relative Dates feature and add fixes.
@volodymyr-chekyrta
https://github.com/user-attachments/assets/f242fae7-b1f5-4c6d-ae11-4212066561e0
This video demonstrates the fix for the calendar access popup issue. Now, tapping outside the alert area will correctly dismiss the alert. Additionally, it shows the resolution of duplicate calendar events after reinstallation and logging in with the same user.
https://github.com/user-attachments/assets/4807beca-51bb-4e08-a032-1b41e6144bd8
This video showcases the fix for the redirection behavior on the dates tab. Tapping on the calendar cell/section now correctly redirects the user to the "Dates & Calendar" screen, matching the behavior implemented on the Android side.
https://github.com/user-attachments/assets/243bc9c5-0cda-48c6-90fe-53ef578542b1
This video demonstrates the fix for the incorrect calendar sync status. After removing calendar access from settings, navigating to the "Dates & Calendar" section now correctly indicates access denied instead of falsely indicating a successful sync.
https://github.com/user-attachments/assets/e3180b5a-52dc-4bc9-ac92-66d275c5e293
The video shows the corrected behavior for unsynced courses. Courses marked for sync now show a clear indication that they will be synced in the next iteration. Issue 7: It includes the fix for displaying an empty screen message when no content is available. Issue 8: The video demonstrates the implementation of toast messages to indicate the success or failure of calendar syncs.
https://github.com/user-attachments/assets/b47d4977-2827-413a-9e17-b9b759b1d71f
This video demonstrates the fix for the sync status section being hidden under tabs and headers on the iPhone SE 3rd generation. The sync status section is now correctly visible and accessible.
@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/7bc7e6c8-cf07-4ba5-b492-6e4a3e81c694
Dark theme
Sync status on Course dates page
⚠️ Pay attention. The feature "Use relative dates" will be implemented in the next round.
Design: https://www.figma.com/design/iZ56YMjbRMShCCDxqrqRrR/Open-edX-Mobile-App-All-Screens-v2.1?node-id=0-1&t=V0qsuJDvWwg9aeBd-0