feat: Add tags to a taxonomy by jesperhodge · Pull Request #2872 · openedx/frontend-app-authoring

jesperhodge · 2026-02-12T15:02:11Z

Description

This addresses Modular-learning #132: Adding functionality to create tags from a taxonomy list.

Go to /authoring/taxonomy, open a taxonomy, and you should be able to create new tags.

Acceptance Criteria

Architecture

The previously used Paragon DataTable is not designed to allow in-line edit functionality or work well with trees / deeply nested table entries. So I used tanstack/react-table directly to build a new tree-table that is editable inline.

AI Usage

To speed things up, I have heavily worked with Github Copilot. I have reviewed all the code carefully, but I want to point that out for awareness when it comes to review. I created pretty exhaustive tests to ensure that the code works as expected.

What is not in scope

Fix pagination: right now a row's subtags may be on different pages, which creates problems. So pagination has been disabled for now.
Column sorting
Highlighting newly created rows

openedx-webhooks · 2026-02-12T15:02:18Z

Thanks for the pull request, @jesperhodge!

This repository is currently maintained by @bradenmacdonald.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Details

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

jesperhodge · 2026-03-03T22:34:04Z

Dear reviewers,
there is a lot to do in this PR, since I had to build a new table for this. The core functionality is done, so I would love to already get an early review and feedback, even though this is unfinished. In the interest of getting this out the door soon.

bradenmacdonald · 2026-03-03T23:05:15Z

Thanks, I'm so happy to see this coming together!

Fix pagination: right now a row's subtags may be on different pages, which creates problems

Any thoughts on how to handle this?

Would it be useful to use infinite scroll instead of pagination, and load sub-tags on demand when their parent tag is expanded?

After successfully saving new tags, there is an internal "Preview" mode that displays changes to the table in-place without refreshing and resorting data in the backend

This doesn't seem to be working. When I add a new top-level tag, it always appears at the top of the list, but when I later refresh, it moves to alphabetical order. I think it should immediately put the tag into the correct position and then "flash" it to highlight where it is in the list.

Bugs:

When I add a new top-level tag then add a sub-tag, the top-level tag incorrectly still says "(0)" subtags until I refresh the page.

Here's a bunch of UX feedback. I know you're probably aware of many of these already, and they don't have to be fixed within this PR necessarily, but it's easier for me to just list them all.

It seems odd that "Expand Row" is only implemented for top-level tags. Can we expand/collapse the sub-tags in the future too?
"Expand Row" button would be better as a chevron up/down icon, like on the standard Paragon Collapsible.
"New Tag" button should be alongside the "Expand All" button
Clicking the "three dots" button on a tag/subtag should open a menu like this instead of revealing a button. Revealing the "Add subtag" button causes the whole table layout to shift.
The "three dots" buttons should all be aligned instead of scattered based on tag depth.
Whichever row you hover over could be highlighted.
If we only allow tags to be three levels deep, there is no need to display "(0)" after grandchild tags, as they can never have children.

bradenmacdonald

Don't have time for a full review now, but here's some initial thoughts.

bradenmacdonald · 2026-03-03T23:12:41Z

+  // The table has a VIEW, DRAFT, and a PREVIEW mode. It starts in VIEW mode.
+  // It switches to DRAFT mode when a user edits or creates a tag.
+  // It switches to PREVIEW mode after saving changes, and only switches to VIEW when
+  // the user refreshes the page, orders a column, or navigates to a different page.
+  // During DRAFT and PREVIEW mode the table makes POST requests and receives
+  // success or failure responses.
+  // However, the table does not refresh to show the updated data from the backend.
+  // This allows us to show the newly created or updated tag in the same place without reordering.


I wonder if we can take a simpler approach. What do you think about having just one "mode", and using optimistic updates to inject any newly-created tags into the correct spot? That way, if/when react-query refetches data from the backend, nothing gets disrupted, and we can keep everything in sync.

(plus a toggle to track whether the user is currently creating a new tag or not)

The modes are mostly necessary to prevent reloading data and alphabetical reloading after the user has successfully saved a new tag, so that new tags and subtags are shown at the top. See my larger comment about this criteria. In terms of optimistic updates, which are there to inject updates even before they have been saved, that would prevent tags to show up in the spot we want, and it doesn't align with our AC, which is to show error messages when the tag does not successfully save and not display the tag optimistically hoping for successful save.

Can I resolve this conversation?

I still think the modes are adding a lot of unnecessary complexity. Couldn't we achieve the same thing without these modes, and a much simpler isCurrentlyEditingTag and lastCreatedTag state that correspond to the "draft mode" and "preview mode" respectively?

If you track those two states, then it doesn't matter if/when React query reloads data, and you don't have to have special case behavior or state transitions, or anything else. You just make sure that if there is currently a lastCreatedTag (you'd track both the tag and its parent ID in the state), then it gets "hidden" from its normal place and rendered directly below its parent. I think that would achieve all the AC you have.

You could also do it with optimistic updates too; just because you're using optimistic updates doesn't mean you can't insert the new tag into the desired place (first on the list) or handle errors when they eventually come through. But in that case, it would be more work to avoid the eventual shift into alphabetical order whenever React query does refresh the list, which is trivially avoided by the state approach I suggested above.

I know you're short on time here so I won't block the PR on this but I do think it should be at least marked as a TODO to explore, as a way of simplifying this.

I love these ideas, and put them into a TODO. I would also like to simplify the table as much as possible.
I'd like to explore these ideas more when I find some time in the future.

One question: are you sure these two ideas make the table less complex? My take at least with states like isCurrentlyEditing is that the more states like this we have, the more complex the situation becomes. My goal with the table modes was to abstract from these details and just make the table behavior easy to predict and very robust, that's why the state machine and the safeguards. The overall TagListTable file is now very short and has little state and few useEffects, making the lifecycle quite simple.

I agree that the modes make things less simple, and if we can get rid of them and simplify I would be glad. But wouldn't either of the suggested solutions introduce more complicated table states?

By the way I'm not meaning to push back on your ideas, I'm just brainstorming to figure out what the best solution would be. I'm principally not opposed to either of your suggestions.

I guess I was hoping it would be much less complicated, and the only differences in behavior would be limited to the NestedRows component, but I'll defer to your opinion on it.

The main thing about lastCreatedTag is it doesn't have any global effect; it just affects the parent of the lastCreatedTag (which you'd store in the state), so that within the NestedRows component, you'd have a check that re-orders a local copy of its childRowsData if parentRowValue == lastCreatedTag.parentValue.

So I'm thinking it would just be a few lines of code in <NestedRows>, but maybe I am missing some other behavior that we need.

Let's say I add a new top-level tax "X-man". Then I want to edit this in place not search for it on some other page or at the bottom. Then I add some child tags and some grand child tags to it, and all the while I don't want things to change their place so I can edit it.
Or I just rename an existing parent tag and then I want to make changes to some of its 200 child tags but not others, and also not be interrupted by constant reordering.
How do we account for that? Isn't a display-only data structure, in this case tagTree, the way to handle it?

Then instead of a singular lastCreatedTag, you'd want an array, tagsEditedThisSession = []. I think it would mostly be the same though - a few lines of code in NestedRows to re-order its children to the top if any of them are in the list. (But all the new tags would be in alphabetical order.)

Or for slightly more complex behavior, you have a tagsEditedThisSession = {[tagValue]: timestamp} data structure, and then you can just make <NestedRows> so that it always re-orders its children first by (look up editTimestamp in tagsEditedThisSession if any) then by (tag value). I think this would give all the properties you have specified without any if statements or modes whatsoever. And as long as the sorting was memoized it would be very performant too. I guess this isn't too different from what you're doing with building a whole separate data structure, but I think it's a simpler approach and easier to understand.

That said, if your tagTree data structure is annotating each tag with additional data that goes beyond its local react state, it's probably necessary. I don't think I've seen any such use case yet though.

not be interrupted by constant reordering

I know neither of us are the product decision makers here, but personally I would strongly prefer a UX that shows the tags in place, and just highlights newly created tags with a NEW label / highlight color. I think that always sorting them into place is more consistent.

I just tested this now in MacOS Finder, and it works exactly how I'd expect the tag tree to work: if you have a folder sorted by name and rename or create a folder, as soon as you give it a name it jumps into place in the sorted order but remains highlighted and focused. This is just sensible IMHO.

Screen.Recording.2026-03-26.at.1.20.05.PM.mov

I like the option we see in your video as well, I have no preference between the two UX designs. I'm just following the Acceptance Criteria in what was communicated to me as "the ideal behavior". I can't do anything about that.

Re: tagsEditedThisSession array - maybe I'm wrong, but seems to make things pretty confusing to check in each branch or table row whether a tag should be rendered there or not depending on this array, when the data that's for rendering the tree says something contradicting it. Sounds to me like instead of an array we should just have a tree data structure that holds the table state and correctly reflects it, so you always know what the state is, what it should be, and when it gets refreshed from the backend?

In terms of complexity, what exactly is it that seems complex to you? I also have the impression that this is overcomplicated, but I cannot pinpoint where. Maybe instead of three modes we can just have a preventDataReload state for the table? It would still need to be clearly shown when the table can reload data and when not, so I was a bit concerned that this will create confusion and bugs, so I went with 3 clear modes instead. But I'm wondering now what can improve here.

I'm mostly concerned with simplicity, readability and separation of concerns here, so the only thing that really sticks out for me is that I would ideally like to have a full separation of concerns between the tags as in tag-list and the tree-table, making the tree-table reusable. And move the tagTree to the tree-table so it doesn't depend on tags.

That was my plan for further improvement, but it's a bit of work, so I couldn't do it within this PR.

jesperhodge · 2026-03-04T16:02:39Z

@bradenmacdonald thanks for the very helpful feedback!

Pagination:
I was thinking of changing that behavior in the backend paginator, as I'd like to paginate by 0th-level tags. While nested tags are included because of the full_depth_threshold parameter, I would expect those not to count towards page size. Do you have any concerns about that? I admit it's a bit weird because they are all in one flat array.

Otherwise, as an alternative, as you said, is to load all child / grandchildren tags when we expand the table. Since we have an "Expand All" functionality, and multiple levels, and I don't want to spawn thousands of requests, I think that would need to be one bulk request. So we have initial (paginated) request for top-level tags, and then one second request filtering for only the top-level tags from the current page and loading them unpaginated together with their children.

Tags ordering / Preview:

When I add a new top-level tag, it always appears at the top of the list, but when I later refresh, it moves to alphabetical order. I think it should immediately put the tag into the correct position and then "flash" it to highlight where it is in the list.

I implemented that as we decided for the ACs. The current way it works is indeed that it always appears at the top of the list, but on refresh moves to alphabetical order. Some of the reasons we landed on these ACs were:

In a typical user workflow, if they are adding a top-level tag, they will probably want to add sub-tags at that time. To make that process easy, the tag is displayed at the top of the list, allowing them to quickly add sub-tags.
When sorted alphabetically, if the new tag is not displayed on the first page of the pagination, they will have to hunt for the new tag and will not see where it goes, even if it flashes in some way, because they are not currently on that page.

I brought your recommendation about positioning and flashing to the team, and we are going to raise this concern to the product owner, Jenna, to get her input.

Bugs:

good find about sub-tags number - going to fix that.

UX:

Thanks for listing these UX things. UX for the table is not implemented at all yet, and I plan to fully finish that right here before marking this PR as ready.

Let me know if you have any questions! Your feedback is very valuable to us.

tbain · 2026-03-04T17:39:00Z

    queryKey: taxonomyQueryKeys.taxonomyTagListPage(taxonomyId, pageIndex, pageSize),
    queryFn: async () => {
-      const { data } = await getAuthenticatedHttpClient().get(apiUrls.tagList(taxonomyId, pageIndex, pageSize));
+      const { data } = await getAuthenticatedHttpClient().get(apiUrls.tagList(taxonomyId, pageIndex, pageSize, 1000));


What is the "1000" here? Do we have a place to make this a constant somewhere?

It's pretty arbitrary, it's the recommendation in the API docs if you want to request all depths (since there's no "infinite"). I'll just add a comment that this fetches full depth if that's fine.

@jesperhodge I agree that we may want to make this a constant in each of the places it's used.

tbain · 2026-03-04T18:24:03Z

          >
            <Layout.Element>
-              <TagListTable taxonomyId={taxonomyId} />
+              <TagListTable taxonomyId={taxonomyId} maxDepth={3} />


Is there a place to move this to a configuration so that if we want to change the depth it doesn't require a code change?

Yes, I'll move that to constants

Done, can we resolve this conversation?

bradenmacdonald · 2026-03-04T18:31:04Z

When sorted alphabetically, if the new tag is not displayed on the first page of the pagination, they will have to hunt for the new tag and will not see where it goes, even if it flashes in some way, because they are not currently on that page.

If we implement "flashing", we could also implement "change to the correct page".

I was thinking of changing that behavior in the backend paginator, as I'd like to paginate by 0th-level tags. While nested tags are included because of the full_depth_threshold parameter, I would expect those not to count towards page size. Do you have any concerns about that? I admit it's a bit weird because they are all in one flat array.

It's fine with me if you add the option to paginate by 0th-level tags, but I think it may need to be optional, in order to preserve the API compatibility.

The current API allows you to quickly load the entire taxonomy into memory by requesting the full depth and as many pages as you need until it's complete, and I think that's another option to consider here unless we think there will be taxonomies too large to performantly display in a react-table.

bradenmacdonald · 2026-03-25T22:55:54Z

+  // The table has a VIEW, DRAFT, and a PREVIEW mode. It starts in VIEW mode.
+  // It switches to DRAFT mode when a user edits or creates a tag.
+  // It switches to PREVIEW mode after saving changes, and only switches to VIEW when
+  // the user refreshes the page, orders a column, or navigates to a different page.
+  // During DRAFT and PREVIEW mode the table makes POST requests and receives
+  // success or failure responses.
+  // However, the table does not refresh to show the updated data from the backend.
+  // This allows us to show the newly created or updated tag in the same place without reordering.


I guess I was hoping it would be much less complicated, and the only differences in behavior would be limited to the NestedRows component, but I'll defer to your opinion on it.

The main thing about lastCreatedTag is it doesn't have any global effect; it just affects the parent of the lastCreatedTag (which you'd store in the state), so that within the NestedRows component, you'd have a check that re-orders a local copy of its childRowsData if parentRowValue == lastCreatedTag.parentValue.

So I'm thinking it would just be a few lines of code in <NestedRows>, but maybe I am missing some other behavior that we need.

jesperhodge · 2026-03-26T22:02:16Z

Hi @bradenmacdonald , I did my very best to address everything.
I don't expect lint and code coverage to all pass yet, but you can re-review and I'll fix any of that meanwhile.

There's some weirdness now with the input autofocus or something, where when you type it will mark the first letter. Maybe that's just me, or a little bug. However this is so minor that I don't want to block the PR, get it merged, and fix that later.

bradenmacdonald · 2026-03-26T22:04:37Z

Thanks @jesperhodge. Once I've approved it and the build is green, shall I go ahead and merge it?

jesperhodge · 2026-03-27T14:25:45Z

Thanks @jesperhodge. Once I've approved it and the build is green, shall I go ahead and merge it?

@bradenmacdonald Yes! Thank you, please merge this. I just merged master into it once to make sure it's all ready for merge. Pipeline was green just before that so it should be fine.

I'll be so happy when this is merged!

bradenmacdonald · 2026-03-27T16:29:47Z

There's some weirdness now with the input autofocus or something, where when you type it will mark the first letter. Maybe that's just me, or a little bug.

I am seeing this too. When I go to add a new tag, and type in a name, the first letter "disappears". It's quite annoying so please try to fix it ASAP as a fast follow, if you have time.

feat: add a new tag from frontend

70bece4

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Feb 12, 2026

openedx-webhooks added this to Contributions Feb 12, 2026

github-project-automation Bot moved this to Needs Triage in Contributions Feb 12, 2026

jesperhodge changed the title ~~feat: Add tags to a taxonomy #132~~ feat: Add tags to a taxonomy Feb 12, 2026

feat: Add table control bar with expand button

e6d6cb7

mphilbrick211 moved this from Needs Triage to Waiting on Author in Contributions Feb 18, 2026

jesperhodge added 15 commits February 19, 2026 13:52

feat: create tags

e70becf

feat: use react-table and get full depth of tags

3d9188b

feat: support nested subrows in tag list table

39b8438

feat: can create new tags with a subtag as parent

ba08de0

feat: show add row conditionally on table depth

f11e813

test: make existing tag list table tests work again

54b1175

test: create tags

de4e8c5

test: generate tests from acceptance criteria

3ea6bed

test: add tests for nested sub-tags and taxonomy editability

0f4dfda

feat: keep table working state with new row at top

15e9de1

feat: add tag tree data structure

ec926db

feat: create tag tree

a617cb8

fix: creating top tags

f5f1ffc

feat: add card style

6021b82

feat: add plus icon

38df0b7

bradenmacdonald reviewed Mar 3, 2026

View reviewed changes

tbain reviewed Mar 4, 2026

View reviewed changes

Comment thread src/taxonomy/tag-list/hooks.ts

kdmccormick requested review from kdmccormick and removed request for ormsbee March 24, 2026 20:18

jesperhodge added 2 commits March 25, 2026 17:19

fix: PR comments and cleanup

b3d25bd

fix: typo

6f2f6b9

jesperhodge requested a review from bradenmacdonald March 25, 2026 21:25

bradenmacdonald added the create-sandbox open-craft-grove should create a sandbox environment from this PR label Mar 25, 2026

fix: types

3a96b58

bradenmacdonald reviewed Mar 25, 2026

View reviewed changes

fix: apply github code review suggestions

f193edb

jesperhodge force-pushed the jhodge/create-tags branch from 538d02e to f193edb Compare March 26, 2026 19:25

jesperhodge added 6 commits March 26, 2026 16:25

fix: PR comments

a8f8297

fix: PR comments

0971da4

fix: tests

825bab3

fix: tests

443221c

fix: PR comment

40f409e

fix: PR comments

4a4deef

jesperhodge requested a review from bradenmacdonald March 26, 2026 21:59

jesperhodge added 2 commits March 27, 2026 09:56

fix: remove unused code

38f87b8

Merge remote-tracking branch 'upstream/master' into jhodge/create-tags

e8820a2

bradenmacdonald approved these changes Mar 27, 2026

View reviewed changes

bradenmacdonald merged commit 2e77e19 into openedx:master Mar 27, 2026
9 of 10 checks passed

github-project-automation Bot moved this from Waiting on Author to Done in Contributions Mar 27, 2026

jesperhodge mentioned this pull request Mar 27, 2026

Fix create tag autofocus openedx/modular-learning#265

Closed

Conversation

jesperhodge commented Feb 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Acceptance Criteria

Architecture

AI Usage

What is not in scope

Uh oh!

openedx-webhooks commented Feb 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jesperhodge commented Mar 3, 2026

Uh oh!

bradenmacdonald commented Mar 3, 2026

Uh oh!

bradenmacdonald left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jesperhodge commented Mar 4, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bradenmacdonald commented Mar 4, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jesperhodge commented Mar 26, 2026

Uh oh!

bradenmacdonald commented Mar 26, 2026

Uh oh!

jesperhodge commented Mar 27, 2026

Uh oh!

bradenmacdonald commented Mar 27, 2026

Uh oh!

Uh oh!

jesperhodge commented Feb 12, 2026 •

edited

Loading

openedx-webhooks commented Feb 12, 2026 •

edited

Loading