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

feat: allow blocks lower than units to be bookmarked #35207

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lms/templates/vert_module.html
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ <h2 class="hd hd-2 unit-title">${unit_title}</h2>
<div class="vert-mod">
% for idx, item in enumerate(items):
% if item['content']:
<div class="vert vert-${idx}" data-id="${item['id']}">
<div class="vert vert-${idx}" data-id="${item['id']}" id="${item['id']}">
${HTML(item['content'])}
</div>
%endif
Expand Down
Original file line number Diff line number Diff line change
@@ -1,20 +1,37 @@
(function(define) {
'use strict';
'use strict';

define(['backbone'], function(Backbone) {
return Backbone.Model.extend({
idAttribute: 'id',
defaults: {
course_id: '',
usage_id: '',
display_name: '',
path: [],
created: ''
},
define(['backbone'], function(Backbone) {
return Backbone.Model.extend({
idAttribute: 'id',
defaults: {
course_id: '',
usage_id: '',
display_name: '',
path: [],
created: ''
},

blockUrl: function() {
return '/courses/' + this.get('course_id') + '/jump_to/' + this.get('usage_id');
}
});
blockUrl: function() {
var path = this.get('path');
var url = '/courses/' + this.get('course_id') + '/jump_to/' + this.get('usage_id');
var params = new URLSearchParams();
var usage_id = this.get('usage_id');
// Confirm that current usage_id does not correspond to current unit
// path contains an array of parent blocks to the bookmarked block.
// Units only have two parents i.e. section and subsections.
// Below condition is only satisfied if a block lower than unit is bookmarked.
if (path.length > 2 && usage_id !== path[path.length - 1]) {
params.append('jumpToId', usage_id);
}
if (params.size > 0) {
// Pass nested block details via query parameters for it to be passed to learning mfe
// The learning mfe should pass it back to unit xblock via iframe url params.
// This would allow us to scroll to the child xblock.
url = url + '?' + params.toString();
}
return url;
}
});
});
}(define || RequireJS.define));
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,12 @@
this.bookmarkId = options.bookmarkId;
this.bookmarked = options.bookmarked;
this.usageId = options.usageId;
if (options.bookmarkedText) {
this.bookmarkedText = options.bookmarkedText;
}
if (options.bookmarkText) {
this.bookmarkText = options.bookmarkText;
}
this.setBookmarkState(this.bookmarked);
},

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,7 @@
component_type: componentType,
component_usage_id: componentUsageId
}
).always(function() {
window.location.href = event.currentTarget.pathname;
});
);
},
Comment on lines -81 to 82
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@navinkarkera I am not able to understand this, how is the navigation happening now?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I think I understand this function is called when we click a bookmark, it's just a way to log it, but it just makes me wonder do we had to do a window.location.href I am just trying to figure the reasoning out of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@farhaanbukhsh My best guess is that it was done to log the event first and then navigate but this way we loose query params. It felt weird to navigate using javascript when it is already handled by the browser link element.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand my only concern was are we missing some cases, but it doesn't seem like it.


/**
Expand Down
Loading