Skip to content
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

Learner dashboard 3.2 #3759

Merged
merged 71 commits into from Aug 28, 2017

Conversation

Projects
None yet
5 participants
@Arunabh98
Copy link
Contributor

commented Aug 17, 2017

Things left -
Add the learner dashboard icons to the search page.
Add the end to end test - I am facing some difficulty in this as this feature is very dynamic.

@Arunabh98

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2017

I have done the necessary changes. Now the Library.js does not have even a single change. The entire functionality is shifted to the playlist service and icons directive and they are being used by the summary tiles for display of icons.

Also, I've tried to include as many tests as possible but omitted tests for functions that just use standard methods like push, indexOf etc.

@seanlip
Copy link
Member

left a comment

Sorry, I can only take a quick look now. But the overall "shape" is good! I suggest getting a UI review; this is close to the finish line now, code-wise.

gets inserted at the given position. Otherwise it gets added at the
end.
Returns:

This comment has been minimized.

Copy link
@seanlip

seanlip Aug 26, 2017

Member

Just use one return value. Please check the rest of the PR.

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Aug 26, 2017

Author Contributor

I have changed this and checked the PR. I found one other occasion where I had to change.

$scope.DEFAULT_EMPTY_TITLE = 'Untitled';
$scope.collectionType = constants.ACTIVITY_TYPE_COLLECTION;

This comment has been minimized.

Copy link
@seanlip

seanlip Aug 26, 2017

Member

This is a constant, so call it $scope.ACTIVITY_TYPE_COLLECTION

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Aug 26, 2017

Author Contributor

Done!

@@ -82,6 +83,7 @@ oppia.directive('explorationSummaryTile', [
$scope, $http,
oppiaDatetimeFormatter, RatingComputationService,
windowDimensionsService) {
$scope.explorationType = constants.ACTIVITY_TYPE_EXPLORATION;

This comment has been minimized.

Copy link
@seanlip

seanlip Aug 26, 2017

Member

Ditto.

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Aug 26, 2017

Author Contributor

Done!


LearnerDashboardActivityIds.prototype.belongsToLearnerDashboardActivities = (
function(
activityId) {

This comment has been minimized.

Copy link
@seanlip

seanlip Aug 26, 2017

Member

You should be able to put this after the line above?

That said, the method name is quite long. Maybe just "includesActivity(activityId)"?

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Aug 26, 2017

Author Contributor

Done!

};

$scope.canActivityBeAddedToLearnerPlaylist = function(
activityId) {

This comment has been minimized.

Copy link
@seanlip

seanlip Aug 26, 2017

Member

Move to end of previous line?

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Aug 26, 2017

Author Contributor

Done!

activityId, activityType));
if (isSuccessfullyAdded) {
/* eslint-disable max-len */
$scope.learnerDashboardActivityIds.addToExplorationLearnerPlaylist(

This comment has been minimized.

Copy link
@seanlip

seanlip Aug 26, 2017

Member

I don't understand why we have "exploration" in the name, when there's a general activity type here...

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Aug 26, 2017

Author Contributor

Fixed!

});
};

var _fetchLearnerDashboardData = function() {

This comment has been minimized.

Copy link
@seanlip

seanlip Aug 26, 2017

Member

Shouldn't this service be maintaining a private copy of the activity ids object? You should only be doing a single fetch.

Or rather: the public API for this service should be something like fetchLearnerDashboardActivityIds(), and the logic in that should be "if data exists, return promise containing data, else make new fetch from backend" or similar.

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Aug 26, 2017

Author Contributor

Ohh I think this is included in this service by mistake. This is actually the service for fetching the data of the learner dashboard in which case this is correct. I have removed this from here. Sorry for this!

@@ -424,6 +544,8 @@ <h1 class="stat-value"><[subscriptionsList.length]></h1>
</div>
</div>


This comment has been minimized.

Copy link
@seanlip

seanlip Aug 26, 2017

Member

Drop unnecessary blank lines

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Aug 26, 2017

Author Contributor

Done!

@@ -673,6 +778,8 @@ <h3 translate="I18N_LEARNER_DASHBOARD_SUGGESTION_TEXT"></h3>
{{ super() }}
<script src="{{TEMPLATE_DIR_PREFIX}}/components/background/BackgroundBannerDirective.js"></script>
<script src="{{TEMPLATE_DIR_PREFIX}}/components/summary_tile/CollectionSummaryTileDirective.js"></script>
<script src="{{TEMPLATE_DIR_PREFIX}}/domain/learner_dashboard/LearnerPlaylistService.js"></script>
<script src="{{TEMPLATE_DIR_PREFIX}}/pages/library/LearnerDashboardIdsBackendApiService.js"></script>

This comment has been minimized.

Copy link
@seanlip

seanlip Aug 26, 2017

Member

This should not go in pages/library. It should go somewhere sensible in domain/. Stuff in pages/ is for just that specific page.

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Aug 26, 2017

Author Contributor

Done!

@Arunabh98

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2017

Addressed review comments.

@jaredsilver I've fixed the issues in dragging elements in most cases. Though sometimes rarely an error might happen. There is an issue about this flakiness opened in ui sortable and after this is sorted even the rare error would not happen.

@@ -137,7 +138,8 @@ <h2 ng-class="{'active': activeGroupIndex === $index}" class="oppia-library-grou
num-views="tile.num_views"
thumbnail-icon-url="tile.thumbnail_icon_url"
thumbnail-bg-color="tile.thumbnail_bg_color"
class="protractor-test-exp-summary-tile">
class="protractor-test-exp-summary-tile"
show-learner-dashboard-icons="userIsLoggedIn">

This comment has been minimized.

Copy link
@seanlip

seanlip Aug 27, 2017

Member

This still shouldn't be passed through. The individual tile should check whether the user is logged in by using e.g. GLOBALS.userIsLoggedIn, and act accordingly. You shouldn't need to go through the library page for that.

The immediate parent of the tile directive might pass a bool attribute along the lines of "show-learner-dashboard-icons-if-possible" (which, if the bool's value is true, conveys that the icons should be shown iff the learner is logged in).

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Aug 27, 2017

Author Contributor

Okay, I'll just change this. Should I keep the attribute the name still as show-learner-dashboard-icons?

This comment has been minimized.

Copy link
@seanlip

seanlip Aug 27, 2017

Member

I think you want the "-if-possible" part I mentioned, for the given reason. Similar comments apply to all the other places where you do this.

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Aug 27, 2017

Author Contributor

Okay.. cool!

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Aug 27, 2017

Author Contributor

I have fixed this. Feels good to see so much modularity! :D

@seanlip
Copy link
Member

left a comment

Hi @Arunabh98, I've taken what should be a final thorough pass. Only small comments, but please address them carefully; some comments repeat comments in previous reviews. After this I'm happy to give LGTM from the technical perspective. Thanks!

@@ -174,9 +174,7 @@
"I18N_INTERACTIONS_SET_INPUT_SUBMIT": "Skicka",
"I18N_LANGUAGE_FOOTER_VIEW_IN": "Visa Oppia på:",
"I18N_LEARNER_DASHBOARD_COMPLETED_SECTION": "Slutförd",
"I18N_LEARNER_DASHBOARD_DELETED_INCOMPLETE_COLLECTIONS": "<[numberDeleted]> av de pågående samlingarna har nyligen raderats. Vi beklagar besväret",

This comment has been minimized.

Copy link
@seanlip

seanlip Aug 27, 2017

Member

Why delete this?

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Aug 27, 2017

Author Contributor

Because it did not have the translation for I18N_LEARNER_DASHBOARD_DELETED_INCOMPLETE_COLLECTION. So if I had included my key. The other one case would just be blank and it maybe the case we would miss to translate this in the future.

Args:
user_id: str. The id of the learner.
Returns:

This comment has been minimized.

Copy link
@seanlip

seanlip Aug 27, 2017

Member

I feel like I shouldn't have to comment on this in three separate passes, but there are still three return values here.

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Aug 27, 2017

Author Contributor

Done!

@@ -34,17 +34,20 @@ oppia.directive('collectionSummaryTile', [
getThumbnailIconUrl: '&thumbnailIconUrl',
getThumbnailBgColor: '&thumbnailBgColor',
isLinkedToEditorPage: '=?isLinkedToEditorPage',
getCategory: '&category'
getCategory: '&category',
isPlaylistMode: '&playlistMode',

This comment has been minimized.

Copy link
@seanlip

seanlip Aug 27, 2017

Member

isInPlaylistMode reads better, I think. Or isPlaylistTile. (But this tile isn't a "mode".)

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Aug 27, 2017

Author Contributor

Done!

getCategory: '&category'
getCategory: '&category',
isPlaylistMode: '&playlistMode',
showLearnerDashboardIcons: '&showLearnerDashboardIcons'

This comment has been minimized.

Copy link
@seanlip

seanlip Aug 27, 2017

Member

I feel like my comments are getting lost somewhere. I thought we were going to do showLearnerDashboardIconsIfPossible (or perhaps showLearnerDashboardIconsIfLoggedIn)?

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Aug 27, 2017

Author Contributor

I have changed this. Maybe it some bug with GitHub. Now that I'm seeing this it is showing this as outdated.

getCategory: '&category'
getCategory: '&category',
isPlaylistMode: '&playlistMode',
showLearnerDashboardIcons: '&showLearnerDashboardIcons'
},
templateUrl: UrlInterpolationService.getDirectiveTemplateUrl(
'/components/summary_tile/' +
'collection_summary_tile_directive.html'),
controller: [
'$scope', 'oppiaDatetimeFormatter',
'COLLECTION_VIEWER_URL', 'COLLECTION_EDITOR_URL', function($scope,

This comment has been minimized.

Copy link
@seanlip

seanlip Aug 27, 2017

Member

Break line after '('. Consider indenting the stuff in '( ... )' by 2 extra spaces to distinguish it from the stuff that's actually in the inner scope.

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Aug 27, 2017

Author Contributor

Done!

@@ -250,6 +251,50 @@ oppia.controller('LearnerDashboard', [
return feedbackThread[$scope.currentFeedbackThreadsSortType];
};

var getPlaylistSortableOptions = function(activityType) {
var windowScrollTop = '';

This comment has been minimized.

Copy link
@seanlip

seanlip Aug 27, 2017

Member

No idea what this means, and slightly surprised it's a string. Maybe add comment to explain?

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Aug 27, 2017

Author Contributor

Done! Found a better solution that prevents even the rare errors I had mentioned earlier.

'/<activityId>', {
activityType: constants.ACTIVITY_TYPE_COLLECTION,
activityType = constants.ACTIVITY_TYPE_COLLECTION;
}

This comment has been minimized.

Copy link
@seanlip

seanlip Aug 27, 2017

Member

else throw Error

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Aug 27, 2017

Author Contributor

Done!

} else if (sectionNameI18nId ===
LEARNER_DASHBOARD_SECTION_I18N_IDS.INCOMPLETE) {
removeActivityUrlPrefix = '/learnerincompleteactivityhandler/';
}

This comment has been minimized.

Copy link
@seanlip

seanlip Aug 27, 2017

Member

else throw Error

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Aug 27, 2017

Author Contributor

Done!

tooltip="Remove"
tooltip-placement="top"></i>
<exploration-summary-tile
style="height: 145px; display: block;"

This comment has been minimized.

Copy link
@seanlip

seanlip Aug 27, 2017

Member

Move this to end of previous line, and revise the indentation accordingly. It's OK if this causes subsequent lines to go > 80 chars.

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Aug 27, 2017

Author Contributor

Done!

tooltip="Remove"
tooltip-placement="top"></i>
<collection-summary-tile
style="height: 145px; display: block;"

This comment has been minimized.

Copy link
@seanlip

seanlip Aug 27, 2017

Member

Ditto.

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Aug 27, 2017

Author Contributor

Done!

@Arunabh98

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2017

Addressed all review comments.

list(str). The titles of the collections to which new explorations have
been added.
The third return valus is the titles of the collections to which new

This comment has been minimized.

Copy link
@seanlip

seanlip Aug 27, 2017

Member

valus --> value

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Aug 27, 2017

Author Contributor

Done!

@@ -35,7 +35,7 @@ oppia.directive('collectionSummaryTile', [
getThumbnailBgColor: '&thumbnailBgColor',
isLinkedToEditorPage: '=?isLinkedToEditorPage',
getCategory: '&category',
isPlaylistMode: '&playlistMode',
isPlaylistTile: '&playlistTile',

This comment has been minimized.

Copy link
@seanlip

seanlip Aug 27, 2017

Member

I think you want the actual directive attr name to also be is-playlist-tile, otherwise it's as though it's naming the playlist tile that's being passed in.

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Aug 27, 2017

Author Contributor

Done!

@@ -44,7 +44,7 @@ oppia.directive('explorationSummaryTile', [
// if it is not specified, it is treated as 0, which means that the
// desktop version of the summary tile is always displayed.
mobileCutoffPx: '@mobileCutoffPx',
isPlaylistMode: '&playlistMode',
isPlaylistTile: '&playlistTile',

This comment has been minimized.

Copy link
@seanlip

seanlip Aug 27, 2017

Member

Ditto.

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Aug 27, 2017

Author Contributor

Done!

LearnerPlaylistService) {
$scope.activityIsActive = true;
function(
$scope, LearnerDashboardIdsBackendApiService,

This comment has been minimized.

Copy link
@seanlip

seanlip Aug 27, 2017

Member

Indent this and next two lines by 2.

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Aug 27, 2017

Author Contributor

Done!

LearnerPlaylistService) {
$scope.activityIsActive = true;

$scope.$watch('activityActive', function(value) {

This comment has been minimized.

Copy link
@seanlip

seanlip Aug 27, 2017

Member

Hm, OK. I would have thought you could pass them in directly, but I'm not too worried. The concern with $watch is performance -- so maybe try adding a console.log() inside it and see how often it gets called. If it doesn't get called too much then what you have is fine.


$scope.$watch('activityActive', function(value) {
$scope.activityIsActive = !$scope.activityIsActive;
$scope.activityIsCurrentlyHoveredOver = (

This comment has been minimized.

Copy link
@seanlip

seanlip Aug 27, 2017

Member

Can we find some way to "set", and not toggle?

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Aug 27, 2017

Author Contributor

Done!

'subsectionName', function($scope, $modalInstance, $http,
sectionNameI18nId, subsectionName) {
'subsectionName', function(
$scope, $modalInstance, $http, sectionNameI18nId,

This comment has been minimized.

Copy link
@seanlip

seanlip Aug 27, 2017

Member

Indent this and the next line by 2.

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Aug 27, 2017

Author Contributor

Done!

text-align: center;
}

@media(max-width: 895px) {

This comment has been minimized.

Copy link
@seanlip

seanlip Aug 27, 2017

Member

Yep, I know, but the existing CSS is fragile. Things get reordered. You might want to consider something like

@media (min-width: 700px) and (max-width: 768px) {

to make it more robust.

@Arunabh98

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2017

Did the CSS changes.

@Arunabh98

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2017

Addressed the review comments.

@seanlip
Copy link
Member

left a comment

LGTM from code perspective. Thanks @Arunabh98!

@jaredsilver
Copy link
Contributor

left a comment

UI (mostly) LGTM. I'll open a couple issues related to the LD project that would be good for folks to address (ideally before the next release), but this is sufficient for merge 👍 Congrats, @Arunabh98!

@seanlip seanlip merged commit bc577c6 into oppia:develop Aug 28, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

giritheja added a commit to giritheja/oppia that referenced this pull request Sep 17, 2017

Merge remote-tracking branch 'upstream/develop' into questions-projec…
…t-m2

* upstream/develop: (27 commits)
  Fixes 3687: batch call for exploration and exploration rights object together. (oppia#3815)
  Fix part oppia#3400 Objective field directive (oppia#3740)
  Fix oppia#3800 Dropdown menu of the ABOUT in navigation bar (oppia#3855)
  Fix oppia#3295 Save draft change list to local storage. (oppia#3584)
  Disable learner dashboard feedback updates send button when no text entered. (oppia#3860)
  Disable save button while uploading audio to server; add Spanish support to audio languages; add audio loading message to learner view (oppia#3856)
  Fix oppia#3834: Introduce backend event models to record statistics (oppia#3841)
  Fix oppia#3852: Fix failing e2e test in develop (oppia#3853)
  Fix oppia#3826: Move validatorsService from app.js to ValidatorsService.js (oppia#3847)
  Update the max RAND limit for ID generation to fit within 32 bits. (oppia#3843)
  Fix oppia#3404: Add default placeholder text for mobile devices  (oppia#3845)
  Fix oppia#2553: Change the "index all explorations" job in the admin dashb… (oppia#3831)
  Add I18N to give up button; change tooltip text. (oppia#3844)
  Fix oppia#3721: Update the release_info script to use LCA and the most recent release tag in order to compile the list of changes, rather than relying on git describe. (oppia#3838)
  show_warning_only_on_button_click (oppia#3833)
  Deprecate splash page experiment (oppia#3829)
  Fixes for end to end working between Oppia and Oppia-ml. (oppia#3824)
  Fix 'Empty path passed in method' error on the collection page. (oppia#3827)
  Learner dashboard 3.2 (oppia#3759)
  📝 Fix oppia#3789 :Space out the profile drop down icons (oppia#3789). (oppia#3817)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.