-
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 #118: Introduce TopicController #174
Conversation
I included most of the team in the review as an FYI for how we can structure domain controllers ahead of implementation. This is a good example of that. I'm happy to submit this with just 1 approval since it's not introducing any logic yet. |
for thumbnails that can be used for lessons.
… introduce-topic-controller-interface
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. Suggested nit changes.
domain/src/test/java/org/oppia/domain/topic/TopicControllerTest.kt
Outdated
Show resolved
Hide resolved
LGTM |
Add method to get concept card data |
Conflicts: model/src/main/proto/topic.proto
Thanks for the reminder @jamesxu0, I didn't quite get to that yet. Keeping this open until I resolve that bit. |
stubbed way), without tests. This also cleans up the voiceover/translation proto dependencies a bit, and migrates Playability over to the ChapterPlayState enum.
@jamesxu0 PTAL. I added concept card support, but ran out of time before I could add tests. Hopefully this is sufficient to unblock you if you branch off of this PR's branch (please let me know if that isn't the case). |
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, this should unblock me for concept card, thanks!
Conflicts: model/src/main/proto/topic.proto
@jamesxu0 I just added tests for getConceptCard in TopicControllerTest. I'd like if you could take a quick glance at them to make sure everything looks good, though I'm fine submitting this as-is. |
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.
Concept Card tests look good! LGTM
Thanks! |
This introduces the interfaces for TopicController, which is also relevant for the #137 since this is providing the source information for both topics & stories. Note that thumbnails for topics & chapters are solved here (stories do not have thumbnails in these views, but do in others: #176). Thumbnails for skills are intentionally left absent, and I don't expect us to have skill thumbnails for the prototype. We should implement a reasonable default for these views in the meantime.
This includes tests to force test replacement once #15 is resolved. This PR includes no real implementation.
Reference page mocks used to determine what information should be provided by this controller: topic page, story page, and concept cards.
This is blocked on #175 hence the base branch.