-
Notifications
You must be signed in to change notification settings - Fork 529
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
Fix #4532: Prevent multiple concept card fragments #4939
Conversation
Checking out the latest code in 'develop'
Update repository
… This prevents opening two dialog fragments on double click.
@masclot is this ready for review or did you open the draft so that people can help with specific parts? Just wanted to make sure that we aren't inadvertently ignoring this. :) |
It is not ready. Since the change is not trivial, I just wanted to show what my idea is, so that I get early comments. @adhiamboperes already commented on chat. |
…wn tag. Allow to switch concept cards by reusing instances instead of creating a new one each time.
- Modify two test concept cards to link to each other; this allows testing the functionality - Only allow one concept card at a time
- Finish implementation - Add unit tests
@adhiamboperes PTAL |
@BenHenning PTAL |
The error in the checks seems to be temporal: # A fatal error has been detected by the Java Runtime Environment: Could anybody rerun them? |
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.
Thanks @masclot! I took a first pass, and I'll be able to provide more specific feedback in the next pass.
- Since you've made changes to scripts/assets files , please have their rationale included in the PR explanation.
- There are some unrelated commits/changes, please remove them.
Unassigning @adhiamboperes since the review is done. |
Hi @masclot, it looks like some changes were requested on this pull request by @adhiamboperes. PTAL. Thanks! |
Addressed the last two conversations. |
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.
Thanks @masclot! Really sorry for the delayed review here.
The PR LGTM, but had 1 nit left and I think the branch needs to be brought up-to-date with the latest develop changes. Please assign back once both are done and I'll go ahead and kick-off the merge.
app/src/sharedTest/java/org/oppia/android/app/topic/conceptcard/ConceptCardFragmentTest.kt
Outdated
Show resolved
Hide resolved
I addressed the comment. Regarding "bringing the branch up-to-date with the latest develop changes", I'm not sure I'm doing it right. In Android Studio, I checked out my develop branch and clicked "Update" on it. I then checked-out the branch for this change and also clicked "Update". Would that be enough? |
I don't think Android Studio -> <branch-name> -> Update does a merge. This menu is what oppia documentation says we should do in Android Studio to update our repository: https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#making-a-local-code-change-using-android-studios-ui-based-github-workflow Instead I did the following in Android Studio:
I hope these steps are good. Please let me know otherwise. |
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.
Thanks @masclot! Re: updating the branch, I think we have instructions in the wiki for this but I can't find them off hand at the moment (@MohitGupta121 might you know?). We generally recommend doing this via Git CLI since it's easier to "debug" when something goes wrong, and I'm less familiar with the Android Studio Git interface.
PR LGTM!
Updating to latest develop & enabling auto-merge. |
Unassigning @BenHenning since they have already approved the PR. |
Hi @masclot, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to ask someone to merge your PR once the CI checks pass and you're happy with it. Thanks! |
Explanation
Fix #4532: only allow one concept card open at a time. If the new new concept card has the same skill id as an existing one, do not open and instead reuse the existing one.
I modified a couple of concept card data for dev so that they link to each other. This change is not necessary for this PR to work, but it allows manually testing the scenario where a concept car links to another.
Current behavior:
multiple concept cards allowed, even with same skill id.webm
New behavior:
ConceptCard deduplication demo.webm
Essential Checklist
For UI-specific PRs only
If your PR includes UI-related changes, then: