-
Notifications
You must be signed in to change notification settings - Fork 527
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 #259 and part of #163: Finish state fragment UI structure #270
Conversation
Separated this in its own PR.
Moved test case to StateFragment
Moved test case to StateFragment,PR suggestions updated
… topic-player-multiple-tabs # Conflicts: # app/build.gradle # app/src/main/AndroidManifest.xml # app/src/main/res/values/styles.xml
… text-input-lowfi-numeric-input-interaction-view-part-2 # Conflicts: # app/src/sharedTest/java/org/oppia/app/player/state/StateFragmentTest.kt
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.
Really great implementation. Mostly nit changes and also, I have addressed some of the issues which I think needs to be fixed in this PR or in High-fi.
Thanks @veena14cs & @rt4914 -- I will address comments tomorrow since it's getting a bit late for me. Rajat, I think most of that should be resolved as part of highfi work, but I agree with hiding the item selection & radio buttons on wrong answers. Let's talk about this during the Android team meeting later. I need a refresher on how Oppia web handles this, too. Also note that the item selection interaction in the exploration itself has a bug--it doesn't display feedback for the correct answer. I think you might have run into this--it's not an issue with the implementation, but the exploration itself. I'll make a note to patch that as part of resolving the open comments. |
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 other than the unresolved comments.
Conflicts: app/src/main/java/org/oppia/app/fragment/FragmentComponent.kt app/src/main/java/org/oppia/app/home/HomeFragmentPresenter.kt app/src/sharedTest/java/org/oppia/app/testing/InputInteractionViewTestActivityTest.kt domain/src/main/java/org/oppia/domain/util/StateRetriever.kt
optimization of all imports.
it was pulled in with earlier, unfinalized changes and we have since realized it's not possible to test the image parser based on the current project setup.
This reverts commit 99783d5.
interesting tests yet.
recent topic & home fragment changes. Also, hide the topic button in the home fragment since the topic is accessible directly via the home fragment tiles.
item selection state for the correct answer so that the player doesn't seem broken.
Thanks everyone for looking at this. There were a large number of style & small fixes in the latest batch, along with a few minor functional changes & build fixes. See the commit log for more detail. @rt4914 I fixed the prototype exploration having no output which led to the player feeling broken. I did not address your other comments around the multiple item selection & multiple choice blocks showing up, or the correct answer box. We'll need to address both in highfi work (I don't want to bloat this PR anymore). Also note that for the correct answer box, that's based on whether an answer is labeled as correct. Most answers aren't labeled as correct in the explorations saved in Oppia's backend, so we'll often not show the box even when it's implemented. We should still implement it though, especially for questions (@jamesxu0 FYI). Please let me know if you want to discuss this further. |
@BenHenning I have taken overview look and also checked all the comments. Almost all replies makes sense and most of my questions will get answered in High-fi work. |
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.
Looks great.
One follow up from talking to @seanlip: we should be showing the completed answers for each interaction, rather than the whole interaction. This works better with the conversational dialog, and it matches Oppia web: https://www.oppia.org/explore/0. Updated my top comment to include this as a future work item. This might make more sense to do as part of follow-up #163 work since it's a bit more structural. I'll note that in #163 as well. |
This fixes #259, and part of #163 (core structure to unblock #164, but no tests yet).
This PR introduces the core functionality to enable all supported prototype interactions (minus number with units) and full exploration functionality, per supported peripheral systems for audio and interactions (including answer classification).
In total, this PR includes:
EphemeralState
from theExplorationProgressController
InteractionObject
s to text answers)Next steps for #163 would be introducing missing tests to guard against regressions. This is blocked on fixing downstream testing issues around swapping dependencies, and fixing existing tests on develop (#295).
Known next steps for #164 include: