-
Notifications
You must be signed in to change notification settings - Fork 16
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: [FC-0047] Calendar synchronization #466
feat: [FC-0047] Calendar synchronization #466
Conversation
Thanks for the pull request, @IvanStepanok! What's next?Please work through the following steps to get your changes ready for engineering review: 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo 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:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. 🔘 Let us know that your PR is ready for review:Who will review my changes?This repository is currently maintained by Where can I find more information?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:
When can I expect my changes to be merged?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:
💡 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 Screen.Recording.2024-06-19.at.11.47.48.mov |
68c0f4d
to
bd46905
Compare
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 It's blocker to review functionality. Simulator.Screen.Recording.-.iPhone.15.-.2024-07-03.at.13.47.54.mp4 |
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.
I hope this will unblock the further review process, thanks! |
I am starting review today 🙌 |
Issue 1: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. Simulator.Screen.Recording.-.iPhone.15.-.2024-07-09.at.17.14.55.mp4 |
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: |
Issue 3: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. DoneIn the Figma designs, there is an option to use relative dates, but I do not see this feature in the current implementation This feature will be implemented in the next MR. It's out of current scope but it is ready.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? You can use my credentials: [email protected] Qwerty123456!If the calendar is unsynced, the following issues are observed when the user accesses the course. 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?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. |
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 |
Issue 4:On dates tab, by tapping on calendar cell/section, user should redirect to Dates & Calendar screen. Redirection behavior is implemented on android side. |
Issue 5:I removed calendar access from Settings and then navigated to the 'Dates & Calendar' section. Despite the console displaying Calendar.Access.Video.mp4 |
Issue 6:Initially, unsynced courses appear under the Simulator.Screen.Recording.-.iPhone.15.-.2024-07-11.at.15.58.43.mp4 |
Issue 8: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. |
Issue 9:On iPhone SE 3rd generation, sync status section hides under the tabs and header. Simulator.Screen.Recording.-.iPhone.SE.3rd.generation.-.2024-07-11.at.16.13.50.mp4 |
@shafqat-muneer Thanks for the review!
Issue 4: – Done ✅ |
Hi @shafqat-muneer, thank you for flagging these issues for design review Issue 6 Issue 7 Issue 8 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
Issue 7
Issue 8
|
@shafqat-muneer, Regarding Issue 3, it appears to be a miscommunication. 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? |
@shafqat-muneer, sure I understand the situation with the release. |
Demo Videos for Issue FixesIssue 1 and 10issue1_10.1.mp4Description: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. Issue 4issue_4.1.mp4Description: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. Issue 5issue_5.1.mp4Description: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. Issue 6, 7, and 8issue_6_7_8.1.mp4Description: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 9issue_9.1.mp4Description: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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
Quick Preview
calendar_axim.mp4
Dark theme
Sync status on Course dates page
Design:
https://www.figma.com/design/iZ56YMjbRMShCCDxqrqRrR/Open-edX-Mobile-App-All-Screens-v2.1?node-id=0-1&t=V0qsuJDvWwg9aeBd-0