[FC-0036] feat: Add content tags tree state + editing #704

yusuf-musleh · 2023-11-21T16:29:07Z

Description

This PR adds the editing functionality for content tags. You are able to select/deselect tags from the dropdowns in each taxonomy and the tags tree structure is updated, requests are sent to the backend, everything gets automatically updated and the state persists.

When a tag is select, all its ancestors automatically become 'implicit' and are unselected and cannot be selected. If an explicit tag is unselected and the parent (or ancestor) no longer has explicit children, all ancestor tags will also be removed.

Supporting information

Related Tickets:

[Tagging] Edit a content item's tags from the tag drawer modular-learning#122

Testing instructions

Run you local dev stack.
Next, navigate to http://localhost:18010/admin/waffle/flag/ and create a new flag: new_studio_mfe.use_tagging_taxonomy_list_page make sure it is active for everyone
If you don't already have sample taxonomy/tags data, follow the instructions in this repo to generate sample data: https://github.com/open-craft/taxonomy-sample-data
Navigate to the sample courses and open the tags drawer for the units
Check that all the taxonomies are shown in the drawer and the applied tags are show
Expand some taxonomies and check that the applied tags shows, implicit and explicit
Click on "Add Tags" button and confirm that the applied tags are reflected in the dropdown selector as well
Confirm that the implicit tags are not clickable
Confirm that when you check/uncheck a tag it is reflected in the tags tree
Confirm that if you check a tag, all its ancestors become implicit
Confirm when you uncheck a tag and it is the only child, all its ancestors get unchecked
Confirm when you uncheck a tag and its has siblings, the ancestors do not get removed
Confirm that "Load More" button functions correctly and loads more tags
Confirm that the tags number gets updated correctly whenever you add/remove tags
Confirm that you can remove tags by clicking on the "x" for explicit tags (same behaviors described above should be observed)

Private-ref: FAL-3530

openedx-webhooks · 2023-11-21T16:29:12Z

Thanks for the pull request, @yusuf-musleh! 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.

src/content-tags-drawer/TagBubble.jsx

src/content-tags-drawer/ContentTagsCollapsible.jsx

bradenmacdonald · 2023-11-22T23:34:39Z

src/content-tags-drawer/ContentTagsCollapsible.jsx

+
+  // Add tag to the tree, and while traversing remove any selected ancestor tags
+  // as they should become implicit
+  const addTags = (tree, tagLineage, selectedTag) => {


Any handler functions should be wrapped in the React.useCallback hook

So this function is being used inside the handler, does it also need to be wrapped in the React.useCallback if the handler itself is wrapped?

Oh, no then it's not necessary... but in that case I would personally just move this function outside of the component altogether to become a top-level function.

BTW the rule generally is if you're using a function as a prop value, it should be wrapped in a hook. The point of a hook like useCallback is that if it's called multiple times, it still returns the exact same function (same instance), and it only re-declares the function if the array of dependencies changes. Otherwise, every time your parent component is rendered, a new function instance is initialized and when you pass it as a prop, react considers it a changed prop (it's an identical function but a different instance, so it's not the same by reference), and then will have to re-render the child component. And as you saw, sometimes the variables captured within your function get out of sync with the components.

Got it, the addTags function uses dispatchers from the useCheckboxSetValues so I left it there, but I moved the other functions outside the component. 7c02136

src/content-tags-drawer/ContentTagsDropDownSelector.jsx

bradenmacdonald · 2023-11-23T18:45:55Z

src/content-tags-drawer/data/apiHooks.jsx

+
+/**
+ * Builds the mutation to update the tags applied to the content object
+ * @returns {import("./types.mjs").UseMutationResult}


Can we delete this line @returns {import("./types.mjs").UseMutationResult} and the custom UseMutationResult type? TypeScript can infer the correct type from the react-query code, and the full type is much more useful/accurate.

With the current code:

Without that line you actually get the full, accurate type:

No need for useContentTaxonomyTagsDataResponse

Also, there is a similar issue above. The return type of useContentTaxonomyTagsData is wrong, and should be set to @returns {import("@tanstack/react-query").UseQueryResult<import("./types.mjs").ContentTaxonomyTagsData>}.

If you do that, you'll see there's no need for the useContentTaxonomyTagsDataResponse function at all.

Instead of

const useContentTaxonomyTagsData = () => { const contentTaxonomyTagsData = useContentTaxonomyTagsDataResponse(contentId); const isContentTaxonomyTagsLoaded = useIsContentTaxonomyTagsDataLoaded(contentId); return { contentTaxonomyTagsData, isContentTaxonomyTagsLoaded }; }; const { contentTaxonomyTagsData, isContentTaxonomyTagsLoaded } = useContentTaxonomyTagsData();

With the accurate types in place, you can just write:

const {data, isSuccess} = useContentTaxonomyTagsData(...);

Or if you want to rename those variables, write it as:

const {data: contentTaxonomyTagsData, isSuccess: isContentTaxonomyTagsLoaded} = useContentTaxonomyTagsData(...);

which is exactly the same thing as the code you have now, but with way less boilerplate.

Interesting! Thanks for the detailed breakdown. That's definitely much simpler and cleaner, I updated all of them to follow this structure: bdc8d30

Nice, much simpler :)

bradenmacdonald · 2023-11-23T18:48:17Z

src/content-tags-drawer/data/apiHooks.jsx

@@ -109,3 +114,13 @@ export const useContentDataResponse = (contentId) => {
 export const useIsContentDataLoaded = (contentId) => (
  useContentData(contentId).status === 'success'
 );
+


In useContentData (above, but I can't comment directly on the right line), the query key is wrong. It needs to include the contentId, so a good query key would be ['contentData', contentId]. Otherwise the data about XBlock B will be read from the cache of XBlock A and users will see the wrong data.

Same for useContentTaxonomyTagsData - the query key needs to include the content ID.

I see, makes sense, I updated it. ec2c842

bradenmacdonald · 2023-11-23T18:58:58Z

src/content-tags-drawer/data/apiHooks.jsx

+ */
+export const useContentTaxonomyTagsMutation = () => (
+  useMutation({
+    mutationFn: ({ contentId, taxonomyId, tags }) => updateContentTaxonomyTags(contentId, taxonomyId, tags),


Here's how to get the right typing for mutationFn:

Remove the incomplete @returns type as mentioned above.

Add a type declaration to mutationFn of @type {import("@tanstack/react-query").MutateFunction<any, any, {contentId: string, taxonomyId: number, tags: string[]}>} as seen in the screenshot below.

Now all the types are accurate - TypeScript can infer the rest automatically. You can see in the screenshot that when you go to call mutate() it knows what arguments you need.

Note: I think taxonomyId should be a number, but you can do it as a string if you want.

Updated as well (also changed ittaxonomyId to a number), bdc8d30

codecov · 2023-11-27T15:25:52Z

Codecov Report

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

Comparison is base (56ad86e) 88.85% compared to head (ae22fb1) 88.85%.

Files	Patch %	Lines
...ntent-tags-drawer/ContentTagsCollapsibleHelper.jsx	95.29%	4 Missing ⚠️
...ontent-tags-drawer/ContentTagsDropDownSelector.jsx	85.00%	3 Missing ⚠️
src/content-tags-drawer/data/apiHooks.jsx	77.77%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #704   +/-   ##
=======================================
  Coverage   88.85%   88.85%           
=======================================
  Files         468      469    +1     
  Lines        7168     7252   +84     
  Branches     1539     1558   +19     
=======================================
+ Hits         6369     6444   +75     
- Misses        772      781    +9     
  Partials       27       27

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

ChrisChV

👍

I tested this: I followed the testing instructions.
I read through the code and considered the security, stability and performance implications of the changes.
I tested that the UI can be used with a keyboard only (tab order, keyboard controls).
Includes tests for bugfixes and/or features added.
Includes documentation

bradenmacdonald

Some minor React hook usage tips...

bradenmacdonald · 2023-11-28T20:32:58Z

src/content-tags-drawer/ContentTagsCollapsible.jsx

+      if (destinationValue && sourceValue && typeof destinationValue === 'object' && typeof sourceValue === 'object') {
+        mergeRecursively(destinationValue, sourceValue);
+      } else {
+        // eslint-disable-next-line no-param-reassign


Nit: I think this lint error was here for good reason. The current implementation is modifying the input objects, which I don't think it should.

const orig = { c: {label: "C", children: {}}, a: {label: "A", children: {}}, b: {label: "B", children: { c: {label: "BC", children: {}}, a: {label: "BA", children: {}}, b: {label: "BB", children: {}}, }}, }; const changed = mergeTrees(orig, orig); console.log(orig);

If you run this (e.g. in your browser console), you'll see that orig gets unexpectedly modified by this, and its b.children have become sorted in-place.

If you run

const orig = { c: {label: "C", children: {}}, a: {label: "A", children: {}}, b: Object.freeze({label: "B", children: { c: {label: "BC", children: {}}, a: {label: "BA", children: {}}, b: {label: "BB", children: {}}, }}), }; const changed = mergeTrees(orig, orig);

you'll get an error when it tries to modify the object b in-place instead of creating a copy of it and modifying that.

It's better to use immutable data structures when possible so although this may not be causing any bugs now, I'd recommend tweaking it to not modify the input objects in any way so that this function behaves more consistently.

@bradenmacdonald Thanks for catching this very intricate bug! Though the main issue wasn't the reassigning of the param, as the linter mentions, since the destination is always copied over (each level of the recursion), however the issue was that the source tree/value was not, hence it was getting modified.

I tried copying over the source value (tree) in the same style (using { ...sourceValue }) however that made it really slow. I realize we have lodash installed, and it has a much more efficient clone method, so I used that instead, and it works great, the input objects no longer get modified. 9fd8c6e

bradenmacdonald · 2023-11-28T20:38:18Z

src/content-tags-drawer/ContentTagsCollapsible.jsx

  const intl = useIntl();
  const {
    id, name, contentTags,
  } = taxonomyAndTagsData;

+  // State to determine whether to update the backend or not


Nit/docs: Just from reading this comment, I'm still unclear on what this state is doing, without reading more of the code. What does it mean "to determine whether to update the backend or not" ?

Got it. It essentially determines whether we should make a call to the update endpoint. I wrote a more descriptive comment: a5087dd

src/content-tags-drawer/ContentTagsCollapsible.jsx

bradenmacdonald · 2023-11-28T20:57:32Z

src/content-tags-drawer/ContentTagsDropDownSelector.jsx

+  // however if it is null that means it is the root, and the apiHooks
+  // would automatically handle it. Later this url is set to the next
+  // page of results (if any)
+  const [fetchUrl, setFetchUrl] = useState(subTagsUrl);


This fetchUrl pattern is a bit weird to me. I would suggest putting in a comment that for the future, we'd probably want to refactor to having a count of how many times the user has clicked "load more" and then use useQueries to load all the pages based on that. No need to refactor now though if this is working.

Yeah fetchUrl feels a bit overloaded and is doing too much at the moment. I agree it warrants a refactor, I added a TODO comment to address it in the future.

bradenmacdonald · 2023-11-28T20:58:32Z

src/content-tags-drawer/TagBubble.jsx

+    if (!implicit && editable) {
+      removeTagHandler(lineage.join(','), false);
+    }
+  }, [implicit, lineage]);


Needs editable and removeTagHandler as dependencies too, since you use those prop variables inside this callback. I'm surprise a react hook linter didn't catch this...

It seems like the exhaustive-deps option is turned off for the linter, not really sure why. That's why it didn't catch it, when I turned it on the linter pointed it out.

https://github.com/openedx/frontend-app-course-authoring/blob/a2dceac62fc13623434ff5324392608d470bc13b/.eslintrc.js#L12

I included the missing dependancies.

bradenmacdonald · 2023-11-29T17:23:36Z

@yusuf-musleh Can you please add [FC-0036] to the PR title?

yusuf-musleh · 2023-11-29T17:41:24Z

@bradenmacdonald Sure thing

bradenmacdonald · 2023-11-29T19:06:45Z

@yusuf-musleh If this is hard to fix, it can be a future PR, but the "add tags" widget is still moving around awkwardly when I click on tags:

recording.mov

bradenmacdonald

Other than the above issue, I'm happy with the code now. Thanks! We'll continue to refine and improve this in future PRs.

yusuf-musleh · 2023-11-30T12:02:08Z

@yusuf-musleh If this is hard to fix, it can be a future PR, but the "add tags" widget is still moving around awkwardly when I click on tags:

recording.mov

@bradenmacdonald hmm, I looked into it a bit. The problem is that the dropdown is "attached" to the "add tags" button, so it moves with it when the new tags are added causing this awkward movement. Keeping it in places however, I feel, would be more awkward as the button moves further away and the dropdown would be "floating" by itself. I think the proper fix would be, as you and @ali-hugo mentioned before, to have the tags added after the dropdown is closed, that should properly fix the issue. I can work on as part of the UI/UX refinements PR.

bradenmacdonald · 2023-11-30T18:14:32Z

@yusuf-musleh Sounds good.

xitij2000

I think not keeping a tree structure, and embracing recursive components might simplify this code quite a bit.

src/content-tags-drawer/ContentTagsCollapsible.jsx

xitij2000 · 2023-12-04T08:34:34Z

src/content-tags-drawer/ContentTagsCollapsible.jsx

+  // State to determine whether the tags are being updating so we can make a call
+  // to the update endpoint to the reflect those changes
+  const [updatingTags, setUpdatingTags] = React.useState(false);
+  const mutation = useContentTaxonomyTagsMutation(contentId, id);


This is not specific to this PR but to most the PRs using useQuery. I think a term like mutation is pretty unclear. I understand that is the preference of this framework, however, it's not clear from this function what it's doing.

I think what might be nice, is to change this name to useContentTaxonomyTagsUpdater, and have that directly require the function to call for the update. i.e.

`

const updateTags = useContentTaxonomyTagsUpdate(contentId, id); ... updateTags({ tags });

That way someone who's not familiar with the framework can still understand what is going on.

Make sense, I've renamed it for clarity, however I kept return object as is, since the mutation object contains methods/properties (such as isError and reset()) that I make use of through out the code.

src/content-tags-drawer/ContentTagsCollapsible.jsx

xitij2000 · 2023-12-04T09:02:39Z

src/content-tags-drawer/ContentTagsCollapsible.jsx

+
+  const handleSelectableBoxChange = React.useCallback((e) => {
+    tagChangeHandler(e.target.value, e.target.checked);
+  });


There is a lot of code in this component before the UI part of this component starts. In situations like this, I think it can be clearer to extract a hook out of it so that the logic and UI are less intertwined.

For example, you could extract the following react hook out of this component:

const useTagCollapsableHelper = (contentId, taxonomyAndTagsData) => { const { id, contentTags, } = taxonomyAndTagsData; // State to determine whether the tags are being updating so we can make a call // to the update endpoint to the reflect those changes const [updatingTags, setUpdatingTags] = React.useState(false); const mutation = useContentTaxonomyTagsMutation(contentId, id); // Keeps track of the content objects tags count (both implicit and explicit) const [contentTagsCount, setContentTagsCount] = React.useState(0); // Keeps track of the tree structure for tags that are add by selecting/unselecting // tags in the dropdowns. const [addedContentTags, setAddedContentTags] = React.useState({}); // To handle checking/unchecking tags in the SelectableBox const [checkedTags, { add, remove, clear }] = useCheckboxSetValues(); // Handles making requests to the update endpoint whenever the checked tags change React.useEffect(() => { // We have this check because this hook is fired when the component first loads // and reloads (on refocus). We only want to make a request to the update endpoint when // the user is updating the tags. if (updatingTags) { setUpdatingTags(false); const tags = checkedTags.map(t => decodeURIComponent(t.split(',').slice(-1))); mutation.mutate({ tags }); } }, [contentId, id, checkedTags]); // This converts the contentTags prop to the tree structure mentioned above const appliedContentTags = React.useMemo(() => { let contentTagsCounter = 0; // Clear all the tags that have not been commited and the checked boxes when // fresh contentTags passed in so the latest state from the backend is rendered setAddedContentTags({}); clear(); // When an error occurs while updating, the contentTags query is invalidated, // hence they will be recalculated, and the mutation should be reset. if (mutation.isError) { mutation.reset(); } const resultTree = {}; contentTags.forEach(item => { let currentLevel = resultTree; item.lineage.forEach((key, index) => { if (!currentLevel[key]) { const isExplicit = index === item.lineage.length - 1; currentLevel[key] = { explicit: isExplicit, children: {}, }; // Populating the SelectableBox with "selected" (explicit) tags const value = item.lineage.map(l => encodeURIComponent(l)).join(','); // eslint-disable-next-line no-unused-expressions isExplicit ? add(value) : remove(value); contentTagsCounter += 1; } currentLevel = currentLevel[key].children; }); }); setContentTagsCount(contentTagsCounter); return resultTree; }, [contentTags, mutation.isError]); // This is the source of truth that represents the current state of tags in // this Taxonomy as a tree. Whenever either the `appliedContentTags` (i.e. tags passed in // the prop from the backed) change, or when the `addedContentTags` (i.e. tags added by // selecting/unselecting them in the dropdown) change, the tree is recomputed. const tagsTree = React.useMemo(() => ( mergeTrees(appliedContentTags, addedContentTags) ), [appliedContentTags, addedContentTags]); // Add tag to the tree, and while traversing remove any selected ancestor tags // as they should become implicit const addTags = (tree, tagLineage, selectedTag) => { const value = []; let traversal = tree; tagLineage.forEach(tag => { const isExplicit = selectedTag === tag; if (!traversal[tag]) { traversal[tag] = { explicit: isExplicit, children: {} }; } else { traversal[tag].explicit = isExplicit; } // Clear out the ancestor tags leading to newly selected tag // as they automatically become implicit value.push(encodeURIComponent(tag)); // eslint-disable-next-line no-unused-expressions isExplicit ? add(value.join(',')) : remove(value.join(',')); traversal = traversal[tag].children; }); }; const tagChangeHandler = React.useCallback((tagSelectableBoxValue, checked) => { const tagLineage = tagSelectableBoxValue.split(',').map(t => decodeURIComponent(t)); const selectedTag = tagLineage.slice(-1)[0]; const addedTree = { ...addedContentTags }; if (checked) { // We "add" the tag to the SelectableBox.Set inside the addTags method addTags(addedTree, tagLineage, selectedTag); } else { // Remove tag from the SelectableBox.Set remove(tagSelectableBoxValue); // We remove them from both incase we are unselecting from an // existing applied Tag or a newly added one removeTags(addedTree, tagLineage); removeTags(appliedContentTags, tagLineage); } setAddedContentTags(addedTree); setUpdatingTags(true); }); return { tagChangeHandler, tagsTree, contentTagsCount, checkedTags }; };

This could be put in a separate file in the component package. It'll help keep the function a bit cleaner.

This is just my preference of course if you feel otherwise do tell.

Agreed, I think splitting it out makes it cleaner, i've done that here: 8d1f7e9

src/content-tags-drawer/ContentTagsCollapsible.jsx

xitij2000 · 2023-12-04T09:15:36Z

src/content-tags-drawer/ContentTagsDropDownSelector.jsx

+
+  const loadMoreTags = useCallback(() => {
+    setFetchUrl(nextPage);
+  }, [nextPage]);


Here you can extract out all the pagination related code into a separate hook called: usePaginatedTaxonomyTagsData. This can be a reusable hook that can handle all the pagination:

const usePaginatedTaxonomyTagsData = (subTagsUrl) => { const [tags, setTags] = useState([]); const [nextPage, setNextPage] = useState(null); // `fetchUrl` is initially `subTagsUrl` to fetch the initial data, // however if it is null that means it is the root, and the apiHooks // would automatically handle it. Later this url is set to the next // page of results (if any) // // TODO: In the future we may need to refactor this to keep track // of the count for how many times the user clicked on "load more" then // use useQueries to load all the pages based on that. const [fetchUrl, setFetchUrl] = useState(subTagsUrl); const { data: taxonomyTagsData, isSuccess: isTaxonomyTagsLoaded } = useTaxonomyTagsData(taxonomyId, fetchUrl); useEffect(() => { if (isTaxonomyTagsLoaded && taxonomyTagsData) { setTags([...tags, ...taxonomyTagsData.results]); setNextPage(taxonomyTagsData.next); } }, [isTaxonomyTagsLoaded, taxonomyTagsData]); const loadMoreTags = useCallback(() => { setFetchUrl(nextPage); }, [nextPage]); return { tags, loadMoreTags, nextPage}; }

You can then just use it as const { tags, nextPage, loadMoreTags} = usePaginatedTaxonomyTagsData(subTagsUrl);

Great suggestion! Since I am refactoring the way I do pagination in the search tags PR I think it makes sense to include this improvement as part of that PR as well.

xitij2000 · 2023-12-04T09:17:00Z

src/content-tags-drawer/ContentTagsDropDownSelector.jsx

+    let traversal = tagsTree;
+    lineage.forEach(t => {
+      // We need to decode the tag to traverse the tree since the lineage value is encoded
+      traversal = traversal[decodeURIComponent(t)]?.children || {};


I'm not sure why decodeURIComponent and encodeURIComponent are being used here. Why not use the direct value?

This is because commas (,) are used as delimiters between tags in the serialized lineage, and tags can contain contain commas. So we encode/decode to avoid any issue there. More discussions: #704 (comment)

xitij2000 · 2023-12-04T09:18:15Z

src/content-tags-drawer/ContentTagsDropDownSelector.scss

+.pgn__selectable_box:disabled,
+.pgn__selectable_box[disabled] {
+  opacity: 1 !important;
+}
+
+.pgn__selectable_box-active {
+  outline: none !important;
+}


Why is this needed?

This is to closer match the Figma design mockups. As the default styling for the selectable box component in Paragon contains borders/greyed out text that differ from the styles in Figma

One thing to note is that often these Figma designs are based on the 2U/edX theme instead of the vanilla theme. So you should try installing that branding package and then seeing what doesn't match.

Additionally, I think we can also start by commenting on the design about changing the design to match figma rather than overriding a component like this.

Note that this will apply to all instances of this component in course-authoring. Which means that other components of course-authoring will stop matching their figma designs.

If the idea is to move paragon in this direction, then that change needs to be made in paragon.

I think it's worth having this discussion and saving this change for later, till the discussion as been had.

Makes sense, I installed the 2U edx theme and style was still off from the Figma designs (note the spacing/alignment in the screenshots are fixed in open-craft#16):

Default Theme without overrides:

2U edx theme without overrides:

I agree, these style changes should be part of the theme or Paragon rather than overriding them here, any idea's who we can discuss this with? Maybe @ali-hugo might have some insights?

For now would it be acceptable to make the overrides more specific by adding the class so it doesn't override all these styles? cae319e

@yusuf-musleh @xitij2000 CC @ali-hugo Also please remember the mockups for this project are not pixel-perfect UX designs. So you don't need to try to match them exactly; if there's an off-the-shelf Paragon component or CSS class that provides the functionality we need, just use it.

But something like you're showing above, where the outline is behind another component, definitely should be fixed.

For questions about how to improve Paragon, I find it best to ask on Slack #wg-frontend

Let's get this PR merged though; we can fine-tune the the appearance in follow-up PRs. We are in MVP mode here and this one holding up other work.

Sounds good, I've asked about it in the #wg-frontend channel.

src/content-tags-drawer/TagBubble.scss

yusuf-musleh · 2023-12-04T14:36:06Z

@xitij2000 Thanks for the review! I've addressed the issues brought up. However regarding the tree structure refactor, I think I'll wait on @bradenmacdonald to weigh in on it in the ticket to see how we should proceed.

yusuf-musleh · 2023-12-07T09:21:24Z

@xitij2000 Friendly reminder for final review.

xitij2000

Looks good and seems to be working. My only concern is the overriding of paragon styles.

xitij2000 · 2023-12-06T12:36:21Z

src/content-tags-drawer/ContentTagsDropDownSelector.scss

+.pgn__selectable_box:disabled,
+.pgn__selectable_box[disabled] {
+  opacity: 1 !important;
+}
+
+.pgn__selectable_box-active {
+  outline: none !important;
+}


One thing to note is that often these Figma designs are based on the 2U/edX theme instead of the vanilla theme. So you should try installing that branding package and then seeing what doesn't match.

Additionally, I think we can also start by commenting on the design about changing the design to match figma rather than overriding a component like this.

Note that this will apply to all instances of this component in course-authoring. Which means that other components of course-authoring will stop matching their figma designs.

If the idea is to move paragon in this direction, then that change needs to be made in paragon.

I think it's worth having this discussion and saving this change for later, till the discussion as been had.

bradenmacdonald · 2023-12-07T17:59:55Z

@yusuf-musleh Could you please squash this to a single commit so @xitij2000 can merge? He prefers that you squash it yourself.

This commit adds the add/remove functionality of content tags where the state is stored and changes are updated in the backend through the API. Changes are reflected in the UI automatically. style: Make custom style overrides more specific fix: ModalPopup appears cut off when top of screen refactor: Extract non-UI logic to helper component refactor: Rename mutation -> updater for clarity chore: Review nits style: Remove hover style for tag bubbles feat: Include implicit tags in tags count badge refactor: Nits from PR review fix: Copy tree to avoid modifying originals test: Add tests for new content tags editing test: Update tests for latest code fix: Fix typing issues with mutations fix: Clear checkboxes when mutation error feat: Add `editable` flag to ContentTagsDrawer feat: Invalidate query refetch content tags count refactor: Simplify react-query hooks + fix types fix: Add missing `contentId` to queryKey refactor: Move util functions outside component fix: Remove arrow from tags dropdown modal feat: "Load more" button to handle paginated tags chore: fix stylelint issues fix: Use Chip instead of Button for TagBubble feat: Add API calls to backend when updating tags feat: Add content tags tree state + editing

yusuf-musleh · 2023-12-08T07:55:21Z

@yusuf-musleh Could you please squash this to a single commit so @xitij2000 can merge? He prefers that you squash it yourself.

@bradenmacdonald Done, commits squashed to one. cc: @xitij2000

openedx-webhooks · 2023-12-08T08:26:00Z

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

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

yusuf-musleh force-pushed the yusuf-musleh/content-tags-drawer-editing branch from 41879d1 to f40f1ea Compare November 22, 2023 15:37

yusuf-musleh commented Nov 22, 2023

View reviewed changes

src/content-tags-drawer/TagBubble.jsx Outdated Show resolved Hide resolved

bradenmacdonald reviewed Nov 22, 2023

View reviewed changes

src/content-tags-drawer/TagBubble.jsx Outdated Show resolved Hide resolved

bradenmacdonald reviewed Nov 22, 2023

View reviewed changes

src/content-tags-drawer/TagBubble.jsx Outdated Show resolved Hide resolved

bradenmacdonald reviewed Nov 22, 2023

View reviewed changes

src/content-tags-drawer/ContentTagsCollapsible.jsx Outdated Show resolved Hide resolved

bradenmacdonald reviewed Nov 22, 2023

View reviewed changes

src/content-tags-drawer/ContentTagsDropDownSelector.jsx Show resolved Hide resolved

yusuf-musleh force-pushed the yusuf-musleh/content-tags-drawer-editing branch from 55d5385 to 5929062 Compare November 23, 2023 12:32

bradenmacdonald reviewed Nov 23, 2023

View reviewed changes

yusuf-musleh force-pushed the yusuf-musleh/content-tags-drawer-editing branch from 14722f9 to 313728b Compare November 26, 2023 13:25

yusuf-musleh changed the title ~~[WIP] feat: Add content tags tree state + editing~~ feat: Add content tags tree state + editing Nov 27, 2023

yusuf-musleh marked this pull request as ready for review November 27, 2023 15:39

yusuf-musleh force-pushed the yusuf-musleh/content-tags-drawer-editing branch from b82ce90 to 63e965c Compare November 28, 2023 15:43

yusuf-musleh mentioned this pull request Nov 28, 2023

[Tagging] Display a unit's tags in the tag drawer openedx/modular-learning#118

Closed

ChrisChV approved these changes Nov 28, 2023

View reviewed changes

bradenmacdonald reviewed Nov 28, 2023

View reviewed changes

yusuf-musleh mentioned this pull request Nov 29, 2023

feat: Bump openedx-learning to 0.3.6 openedx/edx-platform#33839

Merged

yusuf-musleh force-pushed the yusuf-musleh/content-tags-drawer-editing branch from 4bdb8e8 to a5087dd Compare November 29, 2023 11:53

yusuf-musleh changed the title ~~feat: Add content tags tree state + editing~~ [FC-0036] feat: Add content tags tree state + editing Nov 29, 2023

bradenmacdonald approved these changes Nov 29, 2023

View reviewed changes

bradenmacdonald requested a review from xitij2000 November 30, 2023 18:35

xitij2000 reviewed Dec 4, 2023

View reviewed changes

yusuf-musleh mentioned this pull request Dec 4, 2023

[WIP] [FC-0036] Search Content Tags open-craft/frontend-app-authoring#16

Closed

yusuf-musleh force-pushed the yusuf-musleh/content-tags-drawer-editing branch 2 times, most recently from f35727e to 8d1f7e9 Compare December 4, 2023 14:20

This was referenced Dec 5, 2023

[Tagging] UX Refinements from Sandbox Round 1 openedx/modular-learning#157

Closed

style: UX Refinements on tagging components [FC-0036] openedx/edx-platform#33884

Merged

xitij2000 approved these changes Dec 7, 2023

View reviewed changes

yusuf-musleh force-pushed the yusuf-musleh/content-tags-drawer-editing branch from cae319e to ae22fb1 Compare December 8, 2023 07:45

xitij2000 merged commit c9b73a5 into openedx:master Dec 8, 2023
6 checks passed

xitij2000 deleted the yusuf-musleh/content-tags-drawer-editing branch December 13, 2023 04:18

kenclary mentioned this pull request Jan 9, 2024

fix: configure webpack to fix paid powerpaste plugin #783

Merged

[FC-0036] feat: Add content tags tree state + editing #704

[FC-0036] feat: Add content tags tree state + editing #704

Conversation

yusuf-musleh commented Nov 21, 2023 • edited Loading

Description

Supporting information

Testing instructions

openedx-webhooks commented Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bradenmacdonald Nov 23, 2023 • edited Loading

Choose a reason for hiding this comment

No need for useContentTaxonomyTagsDataResponse

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bradenmacdonald Nov 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 27, 2023 • edited Loading

Codecov Report

ChrisChV left a comment

Choose a reason for hiding this comment

bradenmacdonald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bradenmacdonald commented Nov 29, 2023

yusuf-musleh commented Nov 29, 2023

bradenmacdonald commented Nov 29, 2023

bradenmacdonald left a comment

Choose a reason for hiding this comment

yusuf-musleh commented Nov 30, 2023

bradenmacdonald commented Nov 30, 2023

xitij2000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yusuf-musleh commented Dec 4, 2023

yusuf-musleh commented Dec 7, 2023

xitij2000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bradenmacdonald commented Dec 7, 2023

yusuf-musleh commented Dec 8, 2023

openedx-webhooks commented Dec 8, 2023

yusuf-musleh commented Nov 21, 2023 •

edited

Loading

openedx-webhooks commented Nov 21, 2023 •

edited

Loading

bradenmacdonald Nov 23, 2023 •

edited

Loading

bradenmacdonald Nov 23, 2023 •

edited

Loading

codecov bot commented Nov 27, 2023 •

edited

Loading