feat: display all child tags in the "bare bones" taxonomy detail page [FAL-3529] [FC-0036] #703

bradenmacdonald · 2023-11-20T20:31:32Z

This PR has some minor improvements to the "Taxonomy Detail Page".

Load child/grandchild tags consistently. (Depends on fix: Fix bugs in the "get tags" REST API, make it more consistent [FAL-3530] [FC-0036] openedx-learning#119 but it can be merged even before that one is merged.)
Allow expanding/collapsing child tags (if any; otherwise don't show).
Display the # of child tags as a separate column.
Set the title of the page (just said "Course Authoring")

This is not the final UI design and all of this can/will be changed - this just makes sure that users can see all the tags in the taxonomy during our initial round of testing.

Before:

After:

Test Instructions

Note that you need to have openedx/openedx-learning#119 checked out locally or you may see duplicate results in the tag tree.

Private ref: FAL-3529

openedx-webhooks · 2023-11-20T20:31:37Z

Thanks for the pull request, @bradenmacdonald! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

codecov · 2023-11-20T20:42:23Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (a622f8e) 88.62% compared to head (eb1ea74) 88.69%.

Files	Patch %	Lines
src/taxonomy/tag-list/TagListTable.jsx	92.30%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #703      +/-   ##
==========================================
+ Coverage   88.62%   88.69%   +0.06%     
==========================================
  Files         446      446              
  Lines        6851     6875      +24     
  Branches     1465     1469       +4     
==========================================
+ Hits         6072     6098      +26     
+ Misses        754      752       -2     
  Partials       25       25

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

rpenido

LGTM 👍

I read through the code
I followed the test instructions and ran the code locally
I checked for accessibility issues by navigating using the keyboard only
User-facing strings are extracted for translation

I added a nit suggestion about the Title, but to me, it is ok to leave it as it is.

Also, when we click Expand all and have a tag without children, we have an empty row drawn. This is from the DataTable, not our SubTagsExpanded component. We could try to hide this row using CSS, but I wonder if we should put time into this.

To meet the coverage criteria, we need to add tests to the SubTagsExpanded and the new temporary hook.

rpenido · 2023-11-20T21:00:20Z

src/taxonomy/TaxonomyListPage.jsx

@@ -35,6 +38,9 @@ const TaxonomyListPage = () => {

  return (
    <>
+      <Helmet>


Good catch!

rpenido · 2023-11-20T22:19:15Z

src/taxonomy/taxonomy-detail/TaxonomyDetailPage.jsx

@@ -74,6 +79,9 @@ const TaxonomyDetailPage = () => {

  return (
    <>
+      <Helmet>
+        <title>{getPageHeadTitle('', taxonomy.name)}</title>


Feel free to push back on this, but what do you think about adding the Taxonomy header to give some context?

Suggested change

<title>{getPageHeadTitle('', taxonomy.name)}</title>

<title>{getPageHeadTitle(taxonomy.name, intl.formatMessage(taxonomyMessages.headerTitle))}</title>

Sure, done! Though I did it slightly differently. You always want the most specific information to come first in the <title> as that's what users can see most easily.

src/generic/Loading.jsx

xitij2000 · 2023-11-21T05:50:31Z

src/taxonomy/tag-list/TagListTable.jsx

+    <ul style={{ listStyleType: 'none' }}>
+      {subTagsData.data.results.map(tagData => (
+        <li key={tagData.id} style={{ paddingLeft: `${(tagData.depth - 1) * 30}px` }}>
+          {tagData.value} <span className="text-light-900">{tagData.childCount > 0 ? `(${tagData.childCount})` : null}</span>
+        </li>
+      ))}
+    </ul>


If this date is nested, instead of using padding perhaps it can do the following:

const TagList = ({subTagData}) => ( <ul style={{ listStyleType: 'none' }}> {subTagsData.data.results.map(tagData => ( <li key={tagData.id}> {tagData.value} <span className="text-light-900">{tagData.childCount > 0 ? `(${tagData.childCount})` : null}</span> {tagData.children && <TagList subTagData={tagData} />} </li> ))} </ul> )

I don't know if the data is structured that way or can be accessed that way. It will avoid the need to use padding to denote hierarchy.

Yes, I think that's what we'll do eventually. We're basically going to be re-building this whole table in openedx/modular-learning#130 so I didn't bother doing anything too fancy for now. Right now we're just trying to get the basic read-only functionality out the door. The data is currently just in a single array, though the tags are in "tree order" in that array, which is why we can display them this way and why I went with the simplest option.

bradenmacdonald · 2023-11-21T20:22:43Z

Also, when we click Expand all and have a tag without children, we have an empty row drawn. This is from the DataTable, not our SubTagsExpanded component. We could try to hide this row using CSS, but I wonder if we should put time into this.

Yeah, it's not worth spending time on for now. This is just a very temporary solution. We're going to have to build this datatable quite differently to implement tickets like openedx/modular-learning#130 and openedx/modular-learning#128 .

bradenmacdonald · 2023-11-21T20:23:43Z

src/taxonomy/data/api.js

@@ -7,6 +7,7 @@ const getApiBaseUrl = () => getConfig().STUDIO_BASE_URL;
 export const getTaxonomyListApiUrl = (org) => {
  const url = new URL('api/content_tagging/v1/taxonomies/', getApiBaseUrl());
  url.searchParams.append('enabled', 'true');
+  url.searchParams.append('page_size', '500'); // For the tagging MVP, we don't paginate the taxonomy list


@ChrisChV FYI - I included a quick fix here for the pagination issue on the taxonomy list page.

xitij2000 · 2023-11-22T04:05:26Z

@bradenmacdonald Is this PR complete and ready to merge from your side?

bradenmacdonald · 2023-11-22T08:05:51Z

@xitij2000 Yes, thanks!

xitij2000 · 2023-11-22T10:21:50Z

@bradenmacdonald Could you squish the commits and add a commit description?

Also includes: - feat: set <title> on taxonomy list page and taxonomy detail page - fix: display all taxonomies on the list page, even if > 10 - refactor: separate out loading spinner component

bradenmacdonald · 2023-11-22T16:40:51Z

@xitij2000 Sure, done. Is the "Squash and Merge" feature not enabled for this repo?

openedx-webhooks · 2023-11-22T20:14:21Z

@bradenmacdonald 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

xitij2000 · 2023-11-23T04:34:48Z

@bradenmacdonald Is this PR complete and ready to merge from your side?

@xitij2000 Sure, done. Is the "Squash and Merge" feature not enabled for this repo?

It is, however it doesn't generate a good commit message, it merely squishes together all the messages, often adding unnecessary temporary commit messages. It's best for the original author to do this since they'll have best context on what all to include in the message.

…#703) Also includes: - feat: set <title> on taxonomy list page and taxonomy detail page - fix: display all taxonomies on the list page, even if > 10 - refactor: separate out loading spinner component

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Nov 20, 2023

bradenmacdonald mentioned this pull request Nov 20, 2023

feat: display all child tags in the "bare bones" taxonomy detail page [FAL-3529] [FC-0036] open-craft/frontend-app-authoring#10

Closed

bradenmacdonald force-pushed the braden/improve-listing-tags-bare-bones branch from 9f4d892 to ea4df3b Compare November 20, 2023 20:34

rpenido approved these changes Nov 20, 2023

View reviewed changes

xitij2000 reviewed Nov 21, 2023

View reviewed changes

bradenmacdonald commented Nov 21, 2023

View reviewed changes

bradenmacdonald force-pushed the braden/improve-listing-tags-bare-bones branch from c821d2d to 61de124 Compare November 21, 2023 23:38

xitij2000 approved these changes Nov 22, 2023

View reviewed changes

feat: display all child tags in the "bare bones" taxonomy detail page

eb1ea74

Also includes: - feat: set <title> on taxonomy list page and taxonomy detail page - fix: display all taxonomies on the list page, even if > 10 - refactor: separate out loading spinner component

bradenmacdonald force-pushed the braden/improve-listing-tags-bare-bones branch from 61de124 to eb1ea74 Compare November 22, 2023 16:40

xitij2000 merged commit 352ef35 into openedx:master Nov 22, 2023
5 checks passed

bradenmacdonald deleted the braden/improve-listing-tags-bare-bones branch November 22, 2023 21:42

itsjeyd added the core contributor PR author is a Core Contributor (who may or may not have write access to this repo). label Nov 23, 2023

kenclary mentioned this pull request Jan 9, 2024

fix: configure webpack to fix paid powerpaste plugin #783

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: display all child tags in the "bare bones" taxonomy detail page [FAL-3529] [FC-0036] #703

feat: display all child tags in the "bare bones" taxonomy detail page [FAL-3529] [FC-0036] #703

bradenmacdonald commented Nov 20, 2023 •

edited

Loading

openedx-webhooks commented Nov 20, 2023

codecov bot commented Nov 20, 2023 •

edited

Loading

rpenido left a comment •

edited

Loading

rpenido Nov 20, 2023

rpenido Nov 20, 2023

bradenmacdonald Nov 21, 2023

xitij2000 Nov 21, 2023

bradenmacdonald Nov 21, 2023

bradenmacdonald commented Nov 21, 2023

bradenmacdonald Nov 21, 2023

xitij2000 commented Nov 22, 2023

bradenmacdonald commented Nov 22, 2023

xitij2000 commented Nov 22, 2023

bradenmacdonald commented Nov 22, 2023

openedx-webhooks commented Nov 22, 2023

xitij2000 commented Nov 23, 2023

	<title>{getPageHeadTitle('', taxonomy.name)}</title>
	<title>{getPageHeadTitle(taxonomy.name, intl.formatMessage(taxonomyMessages.headerTitle))}</title>

feat: display all child tags in the "bare bones" taxonomy detail page [FAL-3529] [FC-0036] #703

feat: display all child tags in the "bare bones" taxonomy detail page [FAL-3529] [FC-0036] #703

Conversation

bradenmacdonald commented Nov 20, 2023 • edited Loading

Before:

After:

Test Instructions

openedx-webhooks commented Nov 20, 2023

codecov bot commented Nov 20, 2023 • edited Loading

Codecov Report

rpenido left a comment • edited Loading

Choose a reason for hiding this comment

rpenido Nov 20, 2023

Choose a reason for hiding this comment

rpenido Nov 20, 2023

Choose a reason for hiding this comment

bradenmacdonald Nov 21, 2023

Choose a reason for hiding this comment

xitij2000 Nov 21, 2023

Choose a reason for hiding this comment

bradenmacdonald Nov 21, 2023

Choose a reason for hiding this comment

bradenmacdonald commented Nov 21, 2023

bradenmacdonald Nov 21, 2023

Choose a reason for hiding this comment

xitij2000 commented Nov 22, 2023

bradenmacdonald commented Nov 22, 2023

xitij2000 commented Nov 22, 2023

bradenmacdonald commented Nov 22, 2023

openedx-webhooks commented Nov 22, 2023

xitij2000 commented Nov 23, 2023

bradenmacdonald commented Nov 20, 2023 •

edited

Loading

codecov bot commented Nov 20, 2023 •

edited

Loading

rpenido left a comment •

edited

Loading