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: Tag List on Unit page [FC-0036] #33645

Merged
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
4a95e8d
feat: Tag List component structure
ChrisChV Nov 1, 2023
d8e327c
feat: Build view to pass tags to UI
ChrisChV Nov 2, 2023
e36cb84
feat: Building Tag list component on unit page
ChrisChV Nov 3, 2023
b383efe
refactor: Extract tag list as a separated component
ChrisChV Nov 6, 2023
a9f2afc
feat: Render tag children
ChrisChV Nov 7, 2023
e19ff55
feat: Manage tags button added on component menu
ChrisChV Nov 7, 2023
7305ed7
chore: Lint
ChrisChV Nov 7, 2023
d79cf97
style: Nits
ChrisChV Nov 7, 2023
b423507
style: Adding comments in get_unit_tags
ChrisChV Nov 8, 2023
3db38e8
Merge branch 'master' into chris/FAL-3527-tags-on-unit-page
ChrisChV Nov 9, 2023
b96fd33
feat: Extract open and close tagging drawer to utility functions
ChrisChV Nov 9, 2023
a005b34
feat: Open manage tag drawer
ChrisChV Nov 9, 2023
694d164
Merge branch 'master' into chris/FAL-3527-tags-on-unit-page
ChrisChV Nov 13, 2023
6d70b80
fix: Bug with multiple children on the same parent tag
ChrisChV Nov 13, 2023
91c03c1
fix: caret up/down for children tags
ChrisChV Nov 13, 2023
76232ba
style: Typo on comments
ChrisChV Nov 14, 2023
7aa790d
style: Nits on UI
ChrisChV Nov 15, 2023
f19b1e8
Merge branch 'master' into chris/FAL-3527-tags-on-unit-page
ChrisChV Nov 15, 2023
aa12f5c
style: Nit on open taxonomy and tag
ChrisChV Nov 15, 2023
29f3b9c
feat: Add a11y support to tag list component
ChrisChV Nov 16, 2023
d3c7603
style: Nit
ChrisChV Nov 16, 2023
aade38f
Merge branch 'master' into chris/FAL-3527-tags-on-unit-page
ChrisChV Nov 16, 2023
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
83 changes: 80 additions & 3 deletions cms/djangoapps/contentstore/views/component.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,14 @@
from common.djangoapps.student.auth import has_course_author_access
from common.djangoapps.xblock_django.api import authorable_xblocks, disabled_xblocks
from common.djangoapps.xblock_django.models import XBlockStudioConfigurationFlag
from cms.djangoapps.contentstore.toggles import use_new_problem_editor
from cms.djangoapps.contentstore.toggles import (
use_new_problem_editor,
use_tagging_taxonomy_list_page,
)
from openedx.core.lib.xblock_utils import get_aside_from_xblock, is_xblock_aside
from openedx.core.djangoapps.discussions.models import DiscussionsConfiguration
from openedx.core.djangoapps.content_staging import api as content_staging_api
from openedx.core.djangoapps.content_tagging.api import get_content_tags
from xmodule.modulestore.django import modulestore # lint-amnesty, pylint: disable=wrong-import-order
from xmodule.modulestore.exceptions import ItemNotFoundError # lint-amnesty, pylint: disable=wrong-import-order
from ..toggles import use_new_unit_page
Expand Down Expand Up @@ -61,7 +65,7 @@
"editor-mode-button", "upload-dialog",
"add-xblock-component", "add-xblock-component-button", "add-xblock-component-menu",
"add-xblock-component-support-legend", "add-xblock-component-support-level", "add-xblock-component-menu-problem",
"xblock-string-field-editor", "xblock-access-editor", "publish-xblock", "publish-history",
"xblock-string-field-editor", "xblock-access-editor", "publish-xblock", "publish-history", "tag-list",
"unit-outline", "container-message", "container-access", "license-selector", "copy-clipboard-button",
"edit-title-button",
]
Expand Down Expand Up @@ -178,9 +182,14 @@ def container_handler(request, usage_key_string):
prev_url = quote_plus(prev_url) if prev_url else None
next_url = quote_plus(next_url) if next_url else None

show_unit_tags = use_tagging_taxonomy_list_page()
unit_tags = None
if show_unit_tags and is_unit_page:
unit_tags = get_unit_tags(usage_key)

# Fetch the XBlock info for use by the container page. Note that it includes information
# about the block's ancestors and siblings for use by the Unit Outline.
xblock_info = create_xblock_info(xblock, include_ancestor_info=is_unit_page)
xblock_info = create_xblock_info(xblock, include_ancestor_info=is_unit_page, tags=unit_tags)

if is_unit_page:
add_container_page_publishing_info(xblock, xblock_info)
Expand Down Expand Up @@ -216,6 +225,7 @@ def container_handler(request, usage_key_string):
'draft_preview_link': preview_lms_link,
'published_preview_link': lms_link,
'templates': CONTAINER_TEMPLATES,
'show_unit_tags': show_unit_tags,
# Status of the user's clipboard, exactly as would be returned from the "GET clipboard" REST API.
'user_clipboard': user_clipboard,
})
Expand Down Expand Up @@ -598,3 +608,70 @@ def component_handler(request, usage_key_string, handler, suffix=''):
)

return webob_to_django_response(resp)


def get_unit_tags(usage_key):
"""
Get the tags of a Unit and build a json to be read by the UI
"""
# Get content tags from content tagging API
content_tags = get_content_tags(usage_key)
Copy link
Contributor

Choose a reason for hiding this comment

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

@ChrisChV I'm wondering if there is a way to make use of @bradenmacdonald 's updated implementation in the openedx-learning repo that added the lineage information to the object tags and takes care of the grouping under taxonomies as well, rather than re-implementing it here as well we could potentially utilize that as a single source of truth.

Here is a sample of how the data looks like:

{
    "block-v1:SampleTaxonomyOrg1+STC1+2023_1+type@vertical+block@aaf8b8eb86b54281aeeab12499d2cb0b": {
        "taxonomies": [
            {
                "name": "LightCastSkillsTaxonomy",
                "taxonomy_id": 15,
                "editable": true,
                "tags": [
                    {
                        "value": "Administrative Functions",
                        "lineage": [
                            "Administration",
                            "Administrative Support",
                            "Administrative Functions"
                        ]
                    }
                ]
            },
            {
                "name": "OpenCanadaTaxonomy",
                "taxonomy_id": 14,
                "editable": true,
                "tags": [
                    {
                        "value": "Verbal Ability",
                        "lineage": [
                            "Abilities",
                            "Cognitive Abilities",
                            "Communication Abilities",
                            "Verbal Ability"
                        ]
                    }
                ]
            },
            ...
        ]
    }
}

Though I think to make use of that, it seems there needs to be some changes made to the openedx-learning repo, namely extracting the logic out of the ObjectTagsByTaxonomySerializer and exposing a python API that can be used in this repo. Since currently It is only available through the rest api.

Any thoughts on this?

Copy link
Contributor

@bradenmacdonald bradenmacdonald Nov 8, 2023

Choose a reason for hiding this comment

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

My thinking was that it's OK to have this duplicated here, because eventually this will be re-implemented in the course-authoring MFE and fetch data using REST (that's out of scope for us now, but at some point ALL of these Studio pages will be using the new MFE only). When that happens, it can use the logic in openedx-learning.

We should definitely put in a comment stating that when this is moved into an MFE it should be simplified to use the REST API which already provides this grouping + sorting logic. But for now since it's already implemented this way, I think it's fine to leave it like this and get it done sooner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok @bradenmacdonald, that makes sense 👍

@yusuf-musleh Adding another comment, In addition to grouping by taxonomy, it is also necessary to group by parents, to be able to assemble each level of the component.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bradenmacdonald I see, yup, that make sense to me as well!

@ChrisChV Sounds good!


# Group content tags by taxonomy
taxonomy_dict = {}
for content_tag in content_tags:
taxonomy_id = content_tag.taxonomy_id
if taxonomy_id:
if taxonomy_id not in taxonomy_dict:
taxonomy_dict[taxonomy_id] = []
taxonomy_dict[taxonomy_id].append(content_tag)

taxonomy_list = []
total_count = 0

def handle_tag(tags, root_ids, tag, child_tag_id=None):
# Group each tag by parent to build a tree
if tag.id not in tags:
tags[tag.id] = {
'id': tag.id,
'value': tag.value,
'children': [],
}
if child_tag_id:
# Add a child into the children list
tags[tag.id].get('children').append(tags[child_tag_id])
if tag.parent_id is None:
if tag.id not in root_ids:
root_ids.append(tag.id)
else:
# Group all the lineage of this tag
handle_tag(tags, root_ids, tag.parent, tag.id)

# Build a tag tree for each taxonomy
for content_tag_list in taxonomy_dict.values():
tags = {}
root_ids = []

for content_tag in content_tag_list:
if content_tag.tag:
handle_tag(tags, root_ids, content_tag.tag)

taxonomy = content_tag_list[0].taxonomy

if tags:
count = len(tags)
# Add the tree to the taxonomy list
taxonomy_list.append({
'id': taxonomy.id,
'value': taxonomy.name,
'tags': [tags[tag_id] for tag_id in root_ids],
'count': count,
})
total_count += count
Copy link
Contributor

Choose a reason for hiding this comment

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

@ChrisChV It seems like this functionality does not properly handle multiple tags under the same sub-parent.

This is how it is currently showing:

Screen Shot 2023-11-13 at 1 40 14 PM

for the following tags:

Screen Shot 2023-11-13 at 1 38 32 PM

When there aren't multiple tags, its seems to be working as expected:

Screen Shot 2023-11-13 at 1 39 24 PM

Screen Shot 2023-11-13 at 1 38 23 PM

There is a recently created PR in the sample generation repo that provides a better distribution of the tags on components/units. (open-craft/taxonomy-sample-data#3). It can better help with testing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yusuf-musleh Nice catch! I fixed that here 6d70b80


unit_tags = {
'count': total_count,
'taxonomies': taxonomy_list,
}

return unit_tags
Original file line number Diff line number Diff line change
Expand Up @@ -1091,6 +1091,7 @@ def create_xblock_info( # lint-amnesty, pylint: disable=too-many-statements
course=None,
is_concise=False,
summary_configuration=None,
tags=None,
):
"""
Creates the information needed for client-side XBlockInfo.
Expand Down Expand Up @@ -1383,6 +1384,8 @@ def create_xblock_info( # lint-amnesty, pylint: disable=too-many-statements
)
else:
xblock_info["ancestor_has_staff_lock"] = False
if tags is not None:
xblock_info["tags"] = tags

if course_outline:
if xblock_info["has_explicit_staff_lock"]:
Expand Down
4 changes: 4 additions & 0 deletions cms/static/js/models/xblock_info.js
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,10 @@ define(
* True if summary configuration is enabled.
*/
summary_configuration_enabled: null,
/**
* List of tags of the unit. This list is managed by the content_tagging module.
*/
ChrisChV marked this conversation as resolved.
Show resolved Hide resolved
tags: null,
},

initialize: function() {
Expand Down
13 changes: 13 additions & 0 deletions cms/static/js/views/pages/container.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ function($, _, Backbone, gettext, BasePage, ViewUtils, ContainerView, XBlockView
'click .show-actions-menu-button': 'showXBlockActionsMenu',
'click .new-component-button': 'scrollToNewComponentButtons',
'click .paste-component-button': 'pasteComponent',
'click .tags-button': 'openManageTags',
},

options: {
Expand Down Expand Up @@ -97,6 +98,12 @@ function($, _, Backbone, gettext, BasePage, ViewUtils, ContainerView, XBlockView
});
this.viewLiveActions.render();

this.tagListView = new ContainerSubviews.TagList({
el: this.$('.unit-tags'),
model: this.model
});
this.tagListView.render();

this.unitOutlineView = new UnitOutlineView({
el: this.$('.wrapper-unit-overview'),
model: this.model
Expand Down Expand Up @@ -125,6 +132,7 @@ function($, _, Backbone, gettext, BasePage, ViewUtils, ContainerView, XBlockView
xblockView = this.xblockView,
loadingElement = this.$('.ui-loading'),
unitLocationTree = this.$('.unit-location'),
unitTags = this.$('.unit-tags'),
hiddenCss = 'is-hidden';

loadingElement.removeClass(hiddenCss);
Expand All @@ -150,6 +158,7 @@ function($, _, Backbone, gettext, BasePage, ViewUtils, ContainerView, XBlockView
// Refresh the views now that the xblock is visible
self.onXBlockRefresh(xblockView);
unitLocationTree.removeClass(hiddenCss);
unitTags.removeClass(hiddenCss);

// Re-enable Backbone events for any updated DOM elements
self.delegateEvents();
Expand Down Expand Up @@ -409,6 +418,10 @@ function($, _, Backbone, gettext, BasePage, ViewUtils, ContainerView, XBlockView
this.duplicateComponent(this.findXBlockElement(event.target));
},

openManageTags: function() {
// TODO open manage tags drawer
},

showMoveXBlockModal: function(event) {
var xblockElement = this.findXBlockElement(event.target),
parentXBlockElement = xblockElement.parents('.studio-xblock-wrapper'),
Expand Down
122 changes: 121 additions & 1 deletion cms/static/js/views/pages/container_subviews.js
Original file line number Diff line number Diff line change
Expand Up @@ -295,11 +295,131 @@ function($, _, gettext, BaseView, ViewUtils, XBlockViewUtils, MoveXBlockUtils, H
}
});

/**
* TagList displays the tags of a unit.
*/
ChrisChV marked this conversation as resolved.
Show resolved Hide resolved
var TagList = BaseView.extend({
// takes XBlockInfo as a model

events: {
'click .wrapper-tag-header': 'expandTagContainer',
'click .tagging-label': 'expandContentTag',
},

initialize: function() {
BaseView.prototype.initialize.call(this);
this.template = this.loadTemplate('tag-list');
this.model.on('sync', this.onSync, this);
},

onSync: function(model) {
if (ViewUtils.hasChangedAttributes(model, ['tags'])) {
this.render();
}
},

expandTagContainer: function() {
var $content = this.$('.wrapper-tags .wrapper-tag-content'),
$icon = this.$('.wrapper-tags .wrapper-tag-header .icon');

if ($content.hasClass('is-hidden')) {
$content.removeClass('is-hidden');
$icon.addClass('fa-caret-up');
$icon.removeClass('fa-caret-down');
} else {
$content.addClass('is-hidden');
$icon.removeClass('fa-caret-up');
$icon.addClass('fa-caret-down');
}
},

expandContentTag: function(event) {
var contentId = event.target.id,
$content = this.$(`.wrapper-tags .content-tags-${contentId}`),
$icon = this.$(`.wrapper-tags .tagging-label-${contentId} .icon`);

if ($content.hasClass('is-hidden')) {
$content.removeClass('is-hidden');
$icon.addClass('fa-caret-up');
$icon.removeClass('fa-caret-down');
} else {
$content.addClass('is-hidden');
$icon.removeClass('fa-caret-up');
$icon.addClass('fa-caret-down');
}
},
bradenmacdonald marked this conversation as resolved.
Show resolved Hide resolved

renderTagElements: function(tags, depth, parentId) {
const tagListElement = this;
tags.forEach(function(tag) {
const parentElement = document.querySelector(`.content-tags-${parentId}`);
var tagContentElement = document.createElement('div'),
tagValueElement = document.createElement('span');

// Element that contains the tag value and the arrow icon
tagContentElement.style.marginLeft = `${depth}em`;
tagContentElement.className = 'tagging-label';

// Element that contains the tag value
tagValueElement.textContent = tag.value;
tagValueElement.id = `tag-${tag.id}`;

tagContentElement.appendChild(tagValueElement);
parentElement.appendChild(tagContentElement);

if (tag.children.length > 0) {
var tagIconElement = document.createElement('span'),
tagChildrenElement = document.createElement('div');

// Arrow icon
tagIconElement.className = 'icon fa fa-caret-down';
tagIconElement.ariaHidden = 'true';
tagIconElement.id = `tag-${tag.id}`;

// Element that contains the children of this tag
tagChildrenElement.className = `content-tags-tag-${tag.id} is-hidden`;

tagContentElement.appendChild(tagIconElement);
parentElement.appendChild(tagChildrenElement);

// Render children
tagListElement.renderTagElements(tag.children, depth + 1, `tag-${tag.id}`);
}
});
},

renderTags: function() {
if (this.model.get('tags') !== null) {
const taxonomies = this.model.get('tags').taxonomies;
const tagListElement = this;
taxonomies.forEach(function(taxonomy) {
tagListElement.renderTagElements(taxonomy.tags, 1, `tax-${taxonomy.id}`);
});
}
},

render: function() {
HtmlUtils.setHtml(
this.$el,
HtmlUtils.HTML(
this.template({
tags: this.model.get('tags'),
})
)
);

this.renderTags();

return this;
}
});

return {
MessageView: MessageView,
ViewLiveButtonController: ViewLiveButtonController,
Publisher: Publisher,
PublishHistory: PublishHistory,
ContainerAccess: ContainerAccess
ContainerAccess: ContainerAccess,
TagList: TagList
};
}); // end define();
28 changes: 28 additions & 0 deletions cms/static/sass/elements/_controls.scss
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,34 @@
}
}

// inverse primary button
%btn-primary-inverse {
@extend %ui-btn-primary;

background: theme-color("inverse");
border-color: theme-color("primary");
color: theme-color("primary");

&:hover,
&:active {
background: theme-color("primary");
border-color: $uxpl-blue-hover-active;
color: theme-color("inverse");
}

&.current,
&.active {
background: theme-color("primary");
border-color: $uxpl-blue-hover-active;
color: theme-color("inverse");

&:hover,
&:active {
background: theme-color("primary");
}
}
}

// +Secondary Button - Extends
// ====================
// gray secondary button
Expand Down
Loading
Loading