Filter skills #5635

vinitamurthi · 2018-09-09T01:09:51Z

Explanation

Made the skills list scrollable with a fixed height and added a filter for it. This will work for all the skills-list directives.

Checklist

The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes.
The PR explanation includes the words "Fixes #bugnum: ...".
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 follows the style guide.
The PR is assigned to an appropriate reviewer.
- If you're a new contributor, please ask on Gitter for someone to assign a reviewer.
- 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.

…roller

…g merged skills in the untriaged section

vinitamurthi · 2018-09-09T01:11:57Z

Note this builds over the previous merge skills commits so please check only the latest commit i.e. commit number: 11706bf "Filterable skill list"

codecov-io · 2018-09-09T01:53:39Z

Codecov Report

Merging #5635 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #5635   +/-   ##
========================================
  Coverage    45.74%   45.74%           
========================================
  Files          515      515           
  Lines        29954    29954           
  Branches      4522     4522           
========================================
  Hits         13702    13702           
  Misses       16252    16252

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 21d5416...b0271df. Read the comment docs.

tjiang11

Thanks Vinita, LGTM!

aks681

Looks good!

seanlip

Thanks @vinitamurthi! LGTM. Just a couple of notes.

seanlip · 2018-09-13T01:31:37Z

core/controllers/topics_and_skills_dashboard.py

+
+        self.render_json({
+            'merged_into_skill': new_skill_id
+            })


Deindent by 4 to match open braces.

/cc @apb7

seanlip · 2018-09-13T01:32:18Z

core/controllers/topics_and_skills_dashboard.py

+        ]
+        skill_services.update_skill(
+            self.user_id, old_skill['id'], changelist,
+            'Setting merge complete for skill.')


Perhaps: 'Marking the skill as having being merged successfully.'

@vinitamurthi did you see this comment?

Made the change

vinitamurthi · 2018-09-18T00:45:30Z

Once the UI and controller stuff is in, then this one can get merged

oppiabot · 2018-09-20T00:15:26Z

Hi @vinitamurthi. 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!

oppiabot · 2018-09-30T04:48:26Z

Hi @vinitamurthi, I'm going to mark this PR as stale because it hasn't had any updates for 10 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 7 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

kevinlee12 · 2018-10-02T04:00:49Z

Hi @vinitamurthi, do mind the bot, since it will close the PR in 7 days. My comment will reset the timer, so please do provide updates on this PR periodically.

vinitamurthi · 2018-10-05T00:25:54Z

Sorry I was waiting on another PR to submit and thats why this was on hold, it has been merged now and this PR is also ready. @seanlip PTAL !

seanlip

Hi @vinitamurthi -- could you please double-check the diff for this PR and see whether it contains exactly what you want it to? There are a bunch of things which I thought we decided not to do in the last PR, so maybe it's better to redo this one afresh on top of the develop branch.

Thanks!

seanlip · 2018-10-05T01:09:57Z

core/controllers/topics_and_skills_dashboard.py

+        ]
+        skill_services.update_skill(
+            self.user_id, old_skill['id'], changelist,
+            'Setting merge complete for skill.')


@vinitamurthi did you see this comment?

seanlip · 2018-10-05T01:10:08Z

core/templates/dev/head/domain/skill/SkillObjectFactory.js

@@ -119,6 +119,22 @@ oppia.factory('SkillObjectFactory', [
      return this._description;
    };

+    Skill.prototype.setSupersedingSkillId = function(skillId) {


I thought we removed the setters?

Sorry, looks like the merge conflicts were not all removed. It should be clean now

seanlip

Thanks!

vinitamurthi added 9 commits August 31, 2018 15:26

UI + controller change to update superseding skill id

ec3f9fa

Merge remote-tracking branch 'upstream/develop' into merge_skill_cont…

437a262

…roller

Fixed merge conflict

fbbe678

Added functionality to transfer QuestionSkillLinks and to stop showin…

e8034a7

…g merged skills in the untriaged section

Fixed skill services linter error

d58b5d8

Fixed linter errors

fe35c5b

Fixed import order

edca746

Merge remote-tracking branch 'upstream/develop' into filter_skills

9d9f91d

Filterable skill list

11706bf

vinitamurthi requested a review from tjiang11 September 9, 2018 01:09

vinitamurthi requested review from BenHenning and seanlip as code owners September 9, 2018 01:09

seanlip requested a review from aks681 September 9, 2018 05:02

seanlip assigned tjiang11 and aks681 Sep 9, 2018

tjiang11 approved these changes Sep 9, 2018

View reviewed changes

aks681 approved these changes Sep 12, 2018

View reviewed changes

seanlip approved these changes Sep 13, 2018

View reviewed changes

vinitamurthi added 3 commits September 15, 2018 16:11

Merge remote-tracking branch 'upstream/develop' into filter_skills

c3546a3

Fixed review comments

5c993a5

Merge remote-tracking branch 'upstream/develop' into filter_skills

b837f00

oppiabot bot added the PR: don't merge - HAS MERGE CONFLICTS label Sep 20, 2018

oppiabot bot added the stale label Sep 30, 2018

oppiabot bot removed the stale label Oct 2, 2018

vinitamurthi added 4 commits October 4, 2018 17:07

Fix merge conflicts

160a10c

Fixed merge conflicts

839ec6a

Resolved merge conflict

3de904c

Fixed linter error

d401e0f

seanlip requested changes Oct 5, 2018

View reviewed changes

oppiabot bot removed the PR: don't merge - HAS MERGE CONFLICTS label Oct 5, 2018

vinitamurthi added 3 commits October 4, 2018 19:27

Removed old code

6bca715

Linter error fixed

f0ab1d3

Removed old code in object factory

b0271df

seanlip approved these changes Oct 5, 2018

View reviewed changes

seanlip added the PR: LGTM label Oct 5, 2018

kevinlee12 merged commit 2768a1c into oppia:develop Oct 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filter skills #5635

Filter skills #5635

vinitamurthi commented Sep 9, 2018

vinitamurthi commented Sep 9, 2018

codecov-io commented Sep 9, 2018 •

edited

Loading

tjiang11 left a comment

aks681 left a comment

seanlip left a comment

seanlip Sep 13, 2018

seanlip Sep 13, 2018

seanlip Oct 5, 2018

vinitamurthi Oct 5, 2018

vinitamurthi commented Sep 18, 2018

oppiabot bot commented Sep 20, 2018

oppiabot bot commented Sep 30, 2018

kevinlee12 commented Oct 2, 2018

vinitamurthi commented Oct 5, 2018

seanlip left a comment

seanlip Oct 5, 2018

seanlip Oct 5, 2018

vinitamurthi Oct 5, 2018

seanlip left a comment

Filter skills #5635

Filter skills #5635

Conversation

vinitamurthi commented Sep 9, 2018

Explanation

Checklist

vinitamurthi commented Sep 9, 2018

codecov-io commented Sep 9, 2018 • edited Loading

Codecov Report

tjiang11 left a comment

Choose a reason for hiding this comment

aks681 left a comment

Choose a reason for hiding this comment

seanlip left a comment

Choose a reason for hiding this comment

seanlip Sep 13, 2018

Choose a reason for hiding this comment

seanlip Sep 13, 2018

Choose a reason for hiding this comment

seanlip Oct 5, 2018

Choose a reason for hiding this comment

vinitamurthi Oct 5, 2018

Choose a reason for hiding this comment

vinitamurthi commented Sep 18, 2018

oppiabot bot commented Sep 20, 2018

oppiabot bot commented Sep 30, 2018

kevinlee12 commented Oct 2, 2018

vinitamurthi commented Oct 5, 2018

seanlip left a comment

Choose a reason for hiding this comment

seanlip Oct 5, 2018

Choose a reason for hiding this comment

seanlip Oct 5, 2018

Choose a reason for hiding this comment

vinitamurthi Oct 5, 2018

Choose a reason for hiding this comment

seanlip left a comment

Choose a reason for hiding this comment

codecov-io commented Sep 9, 2018 •

edited

Loading