Conversation
|
Thanks for the pull request, @jesperhodge! This repository is currently maintained by 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 approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo 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:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere 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:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
|
Dear reviewers, |
|
Thanks, I'm so happy to see this coming together!
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?
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:
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.
|
bradenmacdonald
left a comment
There was a problem hiding this comment.
Don't have time for a full review now, but here's some initial thoughts.
| // 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. |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Can I resolve this conversation?
|
@bradenmacdonald thanks for the very helpful feedback! Pagination:
Tags ordering / Preview:
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:
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:
UX:
Let me know if you have any questions! Your feedback is very valuable to us. |
src/taxonomy/data/apiHooks.ts
Outdated
| 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)); |
There was a problem hiding this comment.
What is the "1000" here? Do we have a place to make this a constant somewhere?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@jesperhodge I agree that we may want to make this a constant in each of the places it's used.
src/taxonomy/tag-list/hooks.ts
Outdated
| return 'Name is required'; | ||
| } | ||
| if (!TAG_NAME_PATTERN.test(trimmed)) { | ||
| return 'Invalid character in tag name'; |
There was a problem hiding this comment.
Should this return a guide to fix the issue? e.g. "Invalid character in tag name, allowed characters are uppercase letters, lowercase letters, and numbers" or something to that effect? If we have a guide somewhere else on that page aside from this that's fine.
There was a problem hiding this comment.
Good idea, I'll just make that part of the error message. (mentioning forbidden characters ">", ";", and "\tab")
There was a problem hiding this comment.
It looks like the Acceptance Criteria in the ticket just specify that the message indicates that an invalid character was used. Maybe this can be added to the list of things for consideration for fast follow if the language updates are approved through the BA process. cc @thelmick-unicon
There was a problem hiding this comment.
@jesperhodge Sorry, I hadn't refreshed. If you want to go ahead and make the change, it's probably fine, but please indicate in the ticket what the official language ended up being for this.
There was a problem hiding this comment.
This can be on the list of fast follows.
- Can I resolve this conversation?
| private validateNoDuplicateValues(items: TagData[]) { | ||
| const seenValues = new Set<string>(); | ||
| for (const item of items) { | ||
| if (seenValues.has(item.value)) { |
There was a problem hiding this comment.
Does we want this to be case insensitive? So "tuba" would be a duplicate of "Tuba" and "TuBa"?
There was a problem hiding this comment.
No, this should be case sensitive, because:
- I don't know if the backend allows duplicates if they have different cases.
- This function is for error handling when building the tag tree, to tell us what happened, not for input validation.
- The input validation lives in
tag-list/hooks.tsand does not check for duplication at all, because we let the backend handle this when we hit "Save".
There was a problem hiding this comment.
Can we resolve this conversation?
| > | ||
| <Layout.Element> | ||
| <TagListTable taxonomyId={taxonomyId} /> | ||
| <TagListTable taxonomyId={taxonomyId} maxDepth={3} /> |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Yes, I'll move that to constants
There was a problem hiding this comment.
Done, can we resolve this conversation?
If we implement "flashing", we could also implement "change to the correct page".
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. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2872 +/- ##
==========================================
- Coverage 95.51% 95.51% -0.01%
==========================================
Files 1329 1345 +16
Lines 30557 31024 +467
Branches 6925 7059 +134
==========================================
+ Hits 29186 29632 +446
- Misses 1303 1323 +20
- Partials 68 69 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Hi @bradenmacdonald , for now our current designs and acceptance criteria have been aligned on:
|
src/taxonomy/tag-list/messages.ts
Outdated
| }, | ||
| moreActionsForTag: { | ||
| id: 'course-authoring.tag-list.more-actions-for-tag', | ||
| defaultMessage: 'More actions for tag {tagName}', |
There was a problem hiding this comment.
Nitpick: On line 22 there are quotes around the tag name but here there are not. Do we want this to be consistent?
There was a problem hiding this comment.
Done, can we resolve this conversation?
|
Issue: |
Fixed, but please note that there is a backend bug that makes it so when you create a great-grandchild subtag and refresh, it disappears. That is out of scope and subject of openedx/modular-learning#257. |
mgwozdz-unicon
left a comment
There was a problem hiding this comment.
Thank you for all your hard work on this @jesperhodge ! It looks good!


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
Found in Modular-learning #132
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