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.

Sign up for GitHub

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

Jump to bottom

Fix #7021: Add a skill tab in the topic viewer #7287

Merged

aks681 merged 47 commits into oppia:develop from sophiewu6:skill-tab

Aug 14, 2019

Contributor

sophiewu6 commented Aug 4, 2019 •

edited

Loading

Explanation

This PR aims to add a skill tab to display all skills and their skill mastery in the topic.

The skills are sorted based on their skill mastery degrees.

When the user is not logged in, all skill mastery appear to be None.

Checklist

The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
The linter/Karma presubmit checks have passed.
- These should run automatically, but if not, you can manually trigger them locally using python scripts/pre_commit_linter.py and bash scripts/run_frontend_tests.sh.
The PR is made from a branch that's not called "develop".
The PR has an appropriate "PROJECT: ..." label (Please add this label for the first-pass review of the PR).
The PR has an appropriate "CHANGELOG: ..." label (If you are unsure of which label to add, ask the reviewers for guidance).
The PR follows the style guide.
The PR addresses the points mentioned in the codeowner checks for the files/folders changed. (See the codeowner's wiki page.)
The PR is assigned to an appropriate reviewer.
- If you're a new contributor, please ask on Gitter for someone to assign a reviewer and don't tick this checkbox.
- If you're not sure who the appropriate reviewer is, please assign to the issue's "owner" -- see the "talk-to" label on the issue. Do not only request the review but also add the reviewer as an assignee.

sophiewu6 added 30 commits

July 21, 2019 12:56


          user skill mastery backend

0db408e


          lint

975c124


          review changes

afb044a


          lint


          review changes

ea5d418


          lint

dfa9bec


          Merge branch 'develop' of https://github.com/oppia/oppia into fronten…

03edb30

…d-skill-mastery


          review changes

ca1405d


          lint

50b9be7


          edit docstring

e992de5


          Merge branch 'develop' of https://github.com/oppia/oppia into fronten…

19df2d5

…d-skill-mastery


          Merge branch 'user-skill-mastery' of https://github.com/sophiewu6/oppia…

47e5d5d

… into frontend-skill-mastery


          review changes

c6404ce


          Merge branch 'user-skill-mastery' of https://github.com/sophiewu6/oppia…

b449149

… into frontend-skill-mastery


          update and show skill mastery

ad613d2


          lint

709a4df


          lint

00c6031


          travis test error and added log in check

658d7f6


          lint

d986662


          Merge branch 'develop' of https://github.com/oppia/oppia into fronten…

396bdba

…d-skill-mastery


          fix for e2e test

d11e885


          changed file name

a0ab30d


          review changes

029526e


          review changes

7e29fa8


          review changes and small bug fix

fa2eba7


          lint

00031ae


          merge with develop

7b1a384


          added new line at the end of files

ba956fa


          reverted changes to constants.js

a6291aa


          review changes

75c41be

vinitamurthi approved these changes

View reviewed changes

Contributor

vinitamurthi left a comment

Looks good! @aks681 @DubeySandeep please take a look!

vinitamurthi assigned DubeySandeep and aks681 and unassigned vinitamurthi

DubeySandeep approved these changes

View reviewed changes

Member

DubeySandeep left a comment

@sophiewu6, PR looks good to me, left a few simple comments! (Approved!)

Note: I haven't reviewed the UI!

core/templates/dev/head/components/skills-mastery-list/skills-mastery-list.directive.html Outdated

@@ @@ -0,0 +1,107 @@ @@
+              <div class="oppia-skills-list-container">
+                <md-card class="oppia-page-card oppia-long-text">
+                  <div class="skills">

Member

DubeySandeep Aug 10, 2019

name this class such that it starts with oppia- to make it specific. ditto elsewhere

Contributor Author

sophiewu6 Aug 13, 2019

Done.

core/templates/dev/head/components/skills-mastery-list/skills-mastery-list.directive.html Outdated

+                <md-card class="oppia-page-card oppia-long-text">
+                  <div class="skills">
+                    <h3 class="form-heading"> Skills included in this topic </h3>
+                    <h4 ng-if="!$ctrl.userIsLoggedIn" style="color: gray;"> Please login to view your skill mastery. </h4>

Member

DubeySandeep Aug 10, 2019

Add style through classname. (ditto elsewhere.)

Contributor Author

sophiewu6 Aug 13, 2019

Done.

core/templates/dev/head/components/skills-mastery-list/skills-mastery-list.directive.html Outdated

+                        <td> <[$ctrl.getSkillDescriptions()[item.skillId]]> </td>
+                        <td>
+                          <div class="oppia-skill-mastery-container">
+                            <div class="score-bar" style="width: <[$ctrl.getMasteryPercentage($ctrl.getDegreesOfMastery()[item.skillId])]>%; background: <[$ctrl.getColorForMastery($ctrl.getDegreesOfMastery()[item.skillId])]>"></div>

Member

DubeySandeep Aug 10, 2019

this looks complicated, can we use ng-style or some other way to make it look simple?

Contributor Author

sophiewu6 Aug 13, 2019

Done.

core/templates/dev/head/components/skills-mastery-list/skills-mastery-list.directive.html Outdated

+              </div>
+              <style>
+              skills-mastery-list .oppia-page-card {

Member

DubeySandeep Aug 10, 2019

indent by 2. (ditto elsewhere)

Contributor Author

sophiewu6 Aug 13, 2019

Done.

core/templates/dev/head/components/skills-mastery-list/skills-mastery-list.directive.html Outdated

+              <style>
+              skills-mastery-list .oppia-page-card {
+                margin-top: 2%;
+                margin-left: 10%;

Member

DubeySandeep Aug 10, 2019

Arrange in alphabetic order. (Ditto elsewhere.)

Contributor Author

sophiewu6 Aug 13, 2019

Done.

core/templates/dev/head/components/skills-mastery-list/skills-mastery-list.directive.ts Outdated

+                        var ctrl = this;
+                        ctrl.userIsLoggedIn = null;
+                        UserService.getUserInfoAsync().then(function(userInfo) {
+                          ctrl.canCreateCollections = userInfo.canCreateCollections();

Member

DubeySandeep Aug 10, 2019

make it userCanCreateCollection*

Contributor Author

sophiewu6 Aug 13, 2019

Uh, actually this is not needed for this PR. I have removed that.

core/templates/dev/head/pages/topic-viewer-page/topic-viewer-page.directive.html Outdated

@@ @@ -21,6 +26,9 @@ @@
                 <div ng-if="$ctrl.activeTab === 'practice'" class="oppia-practice-tab">
                   <practice-tab topic-name="$ctrl.topicName"></practice-tab>
                 </div>
+                <div ng-if="$ctrl.activeTab === 'skill'" class="oppia-skill-tab">

Member

DubeySandeep Aug 10, 2019

Do we have a constant for "skill" if yes then let's use it as I see "skill" string is used in multiple places in this file?

Contributor Author

sophiewu6 Aug 13, 2019

This part is removed due to @aks681 's comment.

DubeySandeep removed their assignment

aks681 reviewed

View reviewed changes

core/templates/dev/head/components/skills-mastery-list/skills-mastery-list.constants.ts Outdated

+               * @fileoverview Constants for the skills mastery list.
+               */
+              angular.module('oppia').constant('MASTERY_CUTOFF', {

Contributor

aks681 Aug 11, 2019

With the latest develop, this should be split into *constants.ts and *constants.ajs.ts files. You can check some existing files like topic-viewer-domain.constants.ts and topic-viewer-domain.constants.ajs.ts.

Contributor Author

sophiewu6 Aug 13, 2019

Done.

core/templates/dev/head/pages/topic-viewer-page/topic-viewer-page.directive.html Outdated

@@ @@ -13,6 +13,11 @@ @@
                       <a class="oppia-topic-viewer-tabs-text" ng-click="$ctrl.setActiveTab('practice')" translate="I18N_TOPIC_VIEWER_PRACTICE">
                       </a>
                     </span>
+                    <span ng-class="{'oppia-topic-viewer-tabs-active': $ctrl.activeTab === 'skill'}">
+                      <a class="oppia-topic-viewer-tabs-text" ng-click="$ctrl.setActiveTab('skill')">
+                        Skills

Contributor

aks681 Aug 11, 2019 •

edited

Loading

This should be added as a constant to the translation files (en.json and qqq.json), since it is user facing header.

Contributor Author

sophiewu6 Aug 13, 2019

This part is removed.

Contributor

aks681 commented Aug 11, 2019

@seanlip Since we renamed the other tabs to be 'Play', 'Train' and 'Review' (for the subtopic pages tab), should we have a similar name here also, or is 'Skills' fine?

aks681 assigned sophiewu6 and unassigned aks681

Member

seanlip commented Aug 11, 2019

How about having it be part of the Train tab, somehow? Keeping those three tabs at the top (Play/Train/Review) is nice and I think the point where viewing skill mastery is the most relevant would be when you're training. Maybe it can be below the existing "menu of skills to choose from".

Contributor

vinitamurthi commented Aug 11, 2019

@aks681 @seanlip By 'Train' do you mean the current 'Practice' tab? If so, I do like that idea because the current practice tab just has a single button and is pretty empty. @sophiewu6 FYI

Contributor Author

sophiewu6 commented Aug 11, 2019

@aks681 @seanlip I am confused about the 'Play', 'Train', and 'Review' tabs. From what I saw, there are only two tabs ('Story' and "Practice') right now. Also, I didn't see "menu of skills to choose from".

Contributor

aks681 commented Aug 11, 2019

@sophiewu6 Yeah, the old "Story" tab is renamed to "Play" and "Practice" to "Train". I am adding the subtopic pages in #7354, which goes into the "Review" tab. The renaming is done in that PR and should be merged in a few hours.

Member

seanlip commented Aug 11, 2019

@vinitamurthi Yep, that's the one I mean!

Contributor Author

sophiewu6 commented Aug 11, 2019

@aks681 Sounds good. I will make the changes after the PR is merged.

oppiabot bot added the PR: don't merge - HAS MERGE CONFLICTS label

oppiabot bot commented Aug 12, 2019

Hi @sophiewu6. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks!

Contributor

aks681 commented Aug 12, 2019

@sophiewu6 The PR is merged now

sophiewu6 added 3 commits

August 12, 2019 16:34


          merged with develop

de47c02


          review comments

0cd3979


          lint

3107f1e

oppiabot bot removed the PR: don't merge - HAS MERGE CONFLICTS label


          minor change

b5e5497

sophiewu6 assigned aks681 and unassigned sophiewu6

Contributor Author

sophiewu6 commented Aug 13, 2019

@aks681 Comments are addressed. PTAL!

sophiewu6 changed the title ~~Add a skill tab in the topic viewer~~ Fix #7021: Add a skill tab in the topic viewer

aks681 approved these changes

View reviewed changes

aks681 merged commit 36a4e23 into oppia:develop

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet