-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Create a subtopic page viewer #7354
Conversation
Hi, @aks681. This pull request does not have a "CHANGELOG: ..." label as mentioned in the PR checkbox list. Please add this label. PRs without this label will not be merged. If you are unsure of which label to add, please ask the reviewers for guidance. Thanks! |
Hm. @aks681, can you put the content of the subtopic page on a white card please? It looks quite awkward on the grey background. (Also please note merge conflict.) |
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 @aks681, just a couple quick comments, will take another pass later!
class SubtopicPageDataHandler(base.BaseHandler): | ||
"""Manages the data that needs to be displayed to a learner on the | ||
subtopic page. | ||
""" | ||
GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON | ||
|
||
@acl_decorators.can_access_subtopic_viewer_page | ||
def get(self, topic_id, subtopic_id): | ||
def get(self, topic_name, subtopic_id): |
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.
Just checking -- why the switch?
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.
Since this page is accessed from the topic viewer, which has its url as /topic/<topicName>
, it felt right to have the url for the subtopic pages also as /subtopic/<topicName>/<subtopicId>
assets/i18n/qqq.json
Outdated
"I18N_TOPIC_VIEWER_PRACTICE": "Title shown in a tab header in the topic's viewer page. - Clicking on this tab header shows a list of the subtopics that lie within the topic.\n{{Identical|Practice}}", | ||
"I18N_TOPIC_VIEWER_STORY": "Title shown in a tab header in the topic's viewer page. - Clicking on this tab header shows a list of the stories that lie within the topic.\n{{Identical|Story}}", | ||
"I18N_TOPIC_VIEWER_PLAY": "Title shown in a tab header in the topic's viewer page. - Clicking on this tab header shows a list of the stories that lie within the topic.\n{{Identical|Play}}", | ||
"I18N_TOPIC_VIEWER_REVIEW": "Title shown in a tab header in the topic's viewer page. - Clicking on this tab header shows a list of the practice questions that can be used to review the topic.\n{{Identical|Review}}", |
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.
I think you swapped Review and Train accidentally. Train is for the practice questions, and Review is for the subtopic page. (Don't use "review" in the Train description, to avoid confusion.)
Also, why do you ned the "Identical" tags at the end?
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.
It was there in the previous items and so I carried it over, will remove it,
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.
Done
Changed the viewer to have a white background and updated screenshot. |
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 @aks681! Did a full pass.
core/controllers/subtopic_viewer.py
Outdated
class SubtopicPageDataHandler(base.BaseHandler): | ||
"""Manages the data that needs to be displayed to a learner on the | ||
subtopic page. | ||
""" | ||
GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON | ||
|
||
@acl_decorators.can_access_subtopic_viewer_page | ||
def get(self, topic_id, subtopic_id): | ||
def get(self, topic_name, subtopic_id): | ||
"""Handles GET requests.""" |
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.
Add args section to docstring. In this case I think it's important because you want to mention that the given subtopic ID must exist in the subtopic.
Alternatively, take a belt-and-suspenders approach and, in this method, handle the case of the subtopic not existing.
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.
Will add the docstring, though the check for the existence of the subtopic is done in the decorator itself.
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.
Done (adding docstring).
.../pages/subtopic-viewer-page/navbar-breadcrumb/subtopic-viewer-navbar-breadcrumb.directive.ts
Outdated
Show resolved
Hide resolved
@@ -167,8 +167,9 @@ angular.module('oppia').factory('UrlInterpolationService', [ | |||
* Given an story id returns the complete url path to that image. | |||
*/ | |||
getStoryUrl: function(storyId) { | |||
validateResourcePath(storyId); | |||
return '/story' + storyId; | |||
var returnUrl = '/story/' + storyId; |
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.
Consider instead calling interpolateUrl() here, with the appropriate URL template. I think it encodes URI components.
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.
Done, deleted this function and created a new URL template that can be used with interpolateUrl anywhere,
SubtopicViewerBackendApiService, UrlService, FATAL_ERROR_CODES) { | ||
var ctrl = this; | ||
|
||
ctrl.checkMobileView = function() { |
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.
Use a suitable method in WindowDimensionsService for things like this.
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.
Used WindowDimensionsService.getWidth inside this.
core/templates/dev/head/pages/subtopic-viewer-page/subtopic-viewer-page.directive.html
Outdated
Show resolved
Hide resolved
return; | ||
} | ||
|
||
var windowWidth = $(window).width(); |
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.
Don't just copy code wholesale from the library page -- pull out common code into a common directive and reuse it here. (Remember the DRY principle.)
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.
Created issue #7358.
Codecov Report
@@ Coverage Diff @@
## develop #7354 +/- ##
============================================
- Coverage 83% 71.51% -11.49%
============================================
Files 1064 866 -198
Lines 57131 34097 -23034
Branches 2660 2664 +4
============================================
- Hits 47421 24384 -23037
- Misses 9066 9067 +1
- Partials 644 646 +2
|
Codecov Report
@@ Coverage Diff @@
## develop #7354 +/- ##
===========================================
+ Coverage 83.06% 83.09% +0.03%
===========================================
Files 1064 1068 +4
Lines 57708 57822 +114
Branches 2723 2727 +4
===========================================
+ Hits 47931 48043 +112
Misses 9082 9082
- Partials 695 697 +2
|
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.
Just two more small things otherwise LGTM. Thanks!
entity_id: str. The id of the entity. | ||
page_context: str. The context of the page where the asset is | ||
required. | ||
page_identifier: str. The unique identifier for the particular |
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.
Please change this to be more exhaustive. In particular: make a list here of all the allowed entity type, and for each, specify what the entity ID represents.
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.
Done
core/controllers/resources.py
Outdated
entity_id = topic.id | ||
else: |
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.
Make this "elif page_context == one of the allowed entity types"
and add an "else throw InvalidInputException(...)" at the end
(and test these)
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.
Done
@vojtechjelinek @nithusha21 @ankita240796 @DubeySandeep PTAL as codeowners. (This needs to go in ASAP) Thanks! |
Hi @aks681 PTAL at the code coverage checks, thanks! |
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 from code owner's perspective. Thanks!
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.
@aks681, PR, LGTM! (I've left a few comments PTAL!)
raise self.PageNotFoundException | ||
|
||
topic_rights = topic_services.get_topic_rights(topic.id) | ||
if topic_rights is None or not topic_rights.topic_is_published: |
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.
The topic editor or admin cannot see the viewer page until the topic is released/published? (Do we have such preview tab in the subtopic editor page?)
I'm not sure about the viewer page flow so maybe this comment is irrelevant.
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.
Yes, in the sense that once the topic is published, all subtopic pages in it can be viewed. Since the subtopics just contain an RTE, it doesn't require manual publishing for each one.
core/templates/dev/head/components/summary-tile/subtopic-summary-tile.directive.html
Show resolved
Hide resolved
'components/common-layout-directives/common-elements/' + | ||
'background-banner.directive.ts'); | ||
require('directives/angular-html-bind.directive.ts'); | ||
require('directives/mathjax-bind.directive.ts'); |
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.
Add a new line between directive and service import.
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.
You mean directives and domain? Done
core/templates/dev/head/pages/topic-viewer-page/topic-viewer-page.directive.html
Outdated
Show resolved
Hide resolved
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 as a codeowner
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 from codeowners perspective, thanks @aks681!
This PR refines the UI of the topic viewer to some extent and creates the subtopic page viewer, which can be accessed from the topic viewer.
Screenshot (an example with all the RTE elements):