From 55d53852951b90c03a7c7342fa4b9a6003b8b4a1 Mon Sep 17 00:00:00 2001
From: Yusuf Musleh <yusuf.musleh@gmail.com>
Date: Thu, 23 Nov 2023 15:29:44 +0300
Subject: [PATCH] Fix: Use Chip instead of Button for TagBubble

Also refactor tag change handler and wrap it with useCallback
---
 .../ContentTagsCollapsible.jsx                | 17 ++++++++------
 src/content-tags-drawer/TagBubble.jsx         | 23 ++++++++-----------
 src/content-tags-drawer/TagBubble.scss        | 17 +++++++++-----
 src/content-tags-drawer/TagOutlineIcon.jsx    |  1 -
 4 files changed, 31 insertions(+), 27 deletions(-)

diff --git a/src/content-tags-drawer/ContentTagsCollapsible.jsx b/src/content-tags-drawer/ContentTagsCollapsible.jsx
index 789cf18d73..913e0b12a9 100644
--- a/src/content-tags-drawer/ContentTagsCollapsible.jsx
+++ b/src/content-tags-drawer/ContentTagsCollapsible.jsx
@@ -176,7 +176,7 @@ const ContentTagsCollapsible = ({ contentId, taxonomyAndTagsData }) => {
   };
 
   // This converts the contentTags prop to the tree structure mentioned above
-  React.useEffect(() => {
+  React.useMemo(() => {
     const resultTree = {};
 
     contentTags.forEach(item => {
@@ -251,18 +251,17 @@ const ContentTagsCollapsible = ({ contentId, taxonomyAndTagsData }) => {
     }
   };
 
-  const handleSelectableBoxChange = (e) => {
-    // eslint-disable-next-line no-unused-expressions
-    const tagLineage = e.target.value.split(',').map(t => decodeURIComponent(t));
+  const tagChangeHandler = (tagSelectableBoxValue, checked) => {
+    const tagLineage = tagSelectableBoxValue.split(',').map(t => decodeURIComponent(t));
     const selectedTag = tagLineage.slice(-1)[0];
 
     const addedTree = { ...addedContentTags };
-    if (e.target.checked) {
+    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(e.target.value);
+      remove(tagSelectableBoxValue);
 
       // We remove them from both incase we are unselecting from an
       // existing applied Tag or a newly added one
@@ -274,11 +273,15 @@ const ContentTagsCollapsible = ({ contentId, taxonomyAndTagsData }) => {
     setUpdatingTags(true);
   };
 
+  const handleSelectableBoxChange = React.useCallback((e) => {
+    tagChangeHandler(e.target.value, e.target.checked);
+  });
+
   return (
     <div className="d-flex">
       <Collapsible title={name} styling="card-lg" className="taxonomy-tags-collapsible">
         <div key={id}>
-          <ContentTagsTree tagsTree={tagsTree} removeTagHandler={handleSelectableBoxChange} />
+          <ContentTagsTree tagsTree={tagsTree} removeTagHandler={tagChangeHandler} />
         </div>
 
         <div className="d-flex taxonomy-tags-selector-menu">
diff --git a/src/content-tags-drawer/TagBubble.jsx b/src/content-tags-drawer/TagBubble.jsx
index b28c1a912c..84a0cfbe55 100644
--- a/src/content-tags-drawer/TagBubble.jsx
+++ b/src/content-tags-drawer/TagBubble.jsx
@@ -1,6 +1,6 @@
 import React from 'react';
 import {
-  Button,
+  Chip,
 } from '@edx/paragon';
 import { Tag, Close } from '@edx/paragon/icons';
 import PropTypes from 'prop-types';
@@ -11,27 +11,24 @@ const TagBubble = ({
   value, implicit, level, lineage, removeTagHandler,
 }) => {
   const className = `tag-bubble mb-2 ${implicit ? 'implicit' : ''}`;
-  const tagIcon = () => (implicit ? <TagOutlineIcon className="implicit-tag-icon" /> : <Tag />);
 
-  const handleClick = (e) => {
-    if (e.target.value) {
-      e.target.checked = false;
-      removeTagHandler(e);
+  const handleClick = React.useCallback(() => {
+    if (!implicit) {
+      removeTagHandler(lineage.join(','), false);
     }
-  };
+  }, [implicit, lineage]);
 
   return (
     <div style={{ paddingLeft: `${level * 1}rem` }}>
-      <Button
+      <Chip
         className={className}
-        variant="outline-dark"
-        iconBefore={tagIcon}
+        variant="light"
+        iconBefore={!implicit ? Tag : TagOutlineIcon}
         iconAfter={!implicit ? Close : null}
-        onClick={!implicit ? handleClick : null}
-        value={lineage.join(',')}
+        onIconAfterClick={handleClick}
       >
         {value}
-      </Button>
+      </Chip>
     </div>
   );
 };
diff --git a/src/content-tags-drawer/TagBubble.scss b/src/content-tags-drawer/TagBubble.scss
index 281d0fe209..e139d71226 100644
--- a/src/content-tags-drawer/TagBubble.scss
+++ b/src/content-tags-drawer/TagBubble.scss
@@ -1,13 +1,18 @@
-.tag-bubble.btn-outline-dark {
+.tag-bubble.pgn__chip {
   border-color: $light-300;
+  border-style: solid;
+  border-width: 2px;
+  background-color: transparent;
 
   &:hover {
-    color: $white;
+    .pgn__chip__icon-before * {
+      color: $white;
+      fill: $white;
+    }
+    .pgn__chip__label {
+      color: $white;
+    }
     background-color: $dark;
     border-color: $dark;
   }
 }
-
-.implicit > .implicit-tag-icon {
-  color: $dark;
-}
diff --git a/src/content-tags-drawer/TagOutlineIcon.jsx b/src/content-tags-drawer/TagOutlineIcon.jsx
index f817b1f077..7f9d439254 100644
--- a/src/content-tags-drawer/TagOutlineIcon.jsx
+++ b/src/content-tags-drawer/TagOutlineIcon.jsx
@@ -10,7 +10,6 @@ const TagOutlineIcon = (props) => (
     aria-hidden="true"
     {...props}
   >
-    <path d="M0 0h24v24H0V0z" fill="none" />
     <path
       d="m21.41 11.58-9-9C12.05 2.22 11.55 2 11 2H4c-1.1 0-2 .9-2 2v7c0 .55.22 1.05.59 1.42l9 9c.36.36.86.58 1.41.58s1.05-.22 1.41-.59l7-7c.37-.36.59-.86.59-1.41s-.23-1.06-.59-1.42zM13 20.01 4 11V4h7v-.01l9 9-7 7.02z"
     />