feat: add search highlight/expand and "no tags" message [FC-0036] by rpenido · Pull Request #799 · openedx/frontend-app-authoring

rpenido · 2024-01-16T20:54:12Z

Description

This PR makes minor improvements in the search taxonomy UI.

It expands taxonomies that match the search and highlight the search term.
Add "No tag found with search term '....'" message.

Suporting Information

Part of [Tagging] UX Refinements from Sandbox Round 3 modular-learning#176

Testing instructions

Open the tagging drawer and check the search responses to different queries

Private-ref: FAL-3615

openedx-webhooks · 2024-01-16T20:54:17Z

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

rpenido · 2024-01-16T21:00:53Z

+  useEffect(() => {
    setNumPages(1);
-  }
+  }, [searchTerm]);


This hook will be called every time searchTerm changes.

So initially we had this as a useEffect, however we opted for this approach instead after a discussion on a previous PR, since React discourages the use of useEffect in this manner as it causes extra re-renders: https://react.dev/learn/you-might-not-need-an-effect#resetting-all-state-when-a-prop-changes

Thank you for the docs! I didn't know that this was the recommended way.
Reverted: 62022b5

rpenido · 2024-01-16T21:14:29Z

-                style={{
-                  minHeight: '44px',
-                }}
+      {tagPages.isLoading ? (


Changed tagPages from an array to an object that consolidates all pages. More info in the hook

rpenido · 2024-01-16T21:15:56Z

- * @returns {{
- *  hasMorePages: boolean,
- *  tagPages: {
- *    isLoading: boolean,
- *    isError: boolean,
- *    data: TagListData[],
- *  }[],
- * }}


Removing the return type makes it be inferred from the function return

rpenido · 2024-01-16T21:16:39Z

    const preLoadedData = new Map();

-    dataPages.forEach(result => {
+    const newTags = dataPages.map(result => {


This makes the newTags be correctly inferred from the return type.

rpenido · 2024-01-16T21:19:08Z

+  const flatTagPages = {
+    isLoading: tagPages.some(page => page.isLoading),
+    isError: tagPages.some(page => page.isError),
+    isSuccess: tagPages.every(page => page.isSuccess),
+    data: tagPages.flatMap(page => page.data),
+  };
+
+  return { hasMorePages, tagPages: flatTagPages };


I made this change trying to simplify the component side.

Also, I'm not sure why, a new tagPages is being generated every render (I don't think this useMemo is don't its job).

Do you mean every time the searchTerm changes when you say every render? I can't remember if this issue was happening before.

Every render. I got that when I added a useEffect with tagPages in the dependency array and updated an state in the hook. It caused a render loop. But I don't think this is a problem now.

yusuf-musleh

@rpenido Great work! It works well upon testing it. I left a few comments/suggestions.

yusuf-musleh · 2024-01-17T14:35:26Z

+  useEffect(() => {
    setNumPages(1);
-  }
+  }, [searchTerm]);


So initially we had this as a useEffect, however we opted for this approach instead after a discussion on a previous PR, since React discourages the use of useEffect in this manner as it causes extra re-renders: https://react.dev/learn/you-might-not-need-an-effect#resetting-all-state-when-a-prop-changes

yusuf-musleh · 2024-01-17T15:06:46Z

+  const parts = text.split(new RegExp(`(${highlight})`, 'gi'));
+  return (
+    <span>
+      {parts.map((part) => (part.toLowerCase() === highlight.toLowerCase() ? <span className="bg-accent-b">{part}</span> : part))}


This needs to have a unique key attribute, there are warnings in the console when I perform a search.

Fixed here: b574cf5 !

yusuf-musleh · 2024-01-17T15:12:53Z

+  useEffect(() => {
+    if (tagPages.isSuccess) {
+      if (searchTerm) {
+        const expandAll = tagPages.data.reduce(
+          (acc, tagData) => ({
+            ...acc,
+            [tagData.value]: !!tagData.childCount,
+          }),
+          {},
+        );
+        setDropdownStates(expandAll);
+      } else {
+        setDropdownStates({});
+      }
+    }
+  }, [searchTerm, tagPages.isSuccess]);



I could be mistaken if this is the right approach, but I think this could also be achieved without the use of useEffect, if you use the condition from the previous code that checks of the searchTerm has changed and add the logic there. Here is a semi-working solution just to get the idea across:

const [prevSearchTerm, setPrevSearchTerm] = useState(searchTerm); // Reset the page and tags state when search term changes // and store search term to compare if (prevSearchTerm !== searchTerm) { setPrevSearchTerm(searchTerm); setNumPages(1); if (tagPages.isSuccess) { if (searchTerm) { const expandAll = tagPages.data.reduce( (acc, tagData) => ({ ...acc, [tagData.value]: !!tagData.childCount, }), {}, ); setDropdownStates(expandAll); } else { setDropdownStates({}); } } }

This code doesn't correctly expand all the tags, just a demonstration for what I mean, but needs adjustments if this is the route we want to take.

I'm afraid of this path because it could run this code before the API return (tagPages.isSuccess === False) and will not run again when the isSuccess changes.

yusuf-musleh · 2024-01-17T15:30:46Z

+  const flatTagPages = {
+    isLoading: tagPages.some(page => page.isLoading),
+    isError: tagPages.some(page => page.isError),
+    isSuccess: tagPages.every(page => page.isSuccess),
+    data: tagPages.flatMap(page => page.data),
+  };
+
+  return { hasMorePages, tagPages: flatTagPages };


Do you mean every time the searchTerm changes when you say every render? I can't remember if this issue was happening before.

yusuf-musleh · 2024-01-17T15:38:38Z


 import { useTaxonomyTagsData } from './data/apiHooks';

+const HighlightedText = ({ text, highlight }) => {


I like the yellow highlighted text! It familiar to how chrome matches things you search on. However I double checked with the Figma design, it seems like it includes "bolding" of the matching text instead.

Fixed here: 99ec77d

codecov · 2024-01-17T18:31:49Z

Codecov Report

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

Comparison is base (49d4fd4) 90.58% compared to head (74a1684) 90.58%.

Files	Patch %	Lines
...ontent-tags-drawer/ContentTagsDropDownSelector.jsx	90.90%	3 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #799   +/-   ##
=======================================
  Coverage   90.58%   90.58%           
=======================================
  Files         511      511           
  Lines        8879     8902   +23     
  Branches     1912     1919    +7     
=======================================
+ Hits         8043     8064   +21     
- Misses        811      813    +2     
  Partials       25       25

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

rpenido · 2024-01-18T13:18:34Z

Could you take another look here @yusuf-musleh?

yusuf-musleh

👍 @rpenido Looks good to me!

I tested this: Testing it out locally, its working well
I read through the code
I checked for accessibility issues
Includes documentation
~~I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository.~~

xitij2000

Looks good!

rpenido · 2024-01-31T22:48:42Z

Rebased @xitij2000!

xitij2000 · 2024-02-02T06:13:28Z

Rebased @xitij2000!

One small thing, I should have noticed this before. I don't think "fix" is the correct category for this change. It is definitely a feature. Please update the PR title and commit message, and I will merge it. Sorry for not noticing this sooner.

rpenido · 2024-02-02T12:34:50Z

You're right @xitij2000! Thank you!
Changed the title and squashed the commits changing the messsage.

openedx-webhooks · 2024-02-02T15:09:34Z

@rpenido 🎉 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 Jan 16, 2024

rpenido commented Jan 16, 2024

View reviewed changes

rpenido marked this pull request as draft January 16, 2024 21:19

rpenido requested a review from yusuf-musleh January 16, 2024 21:19

yusuf-musleh reviewed Jan 17, 2024

View reviewed changes

rpenido force-pushed the rpenido/fal-3615-ux-refinements-from-sandbox-round-3 branch 2 times, most recently from fa54099 to 1d44c8a Compare January 17, 2024 18:23

rpenido force-pushed the rpenido/fal-3615-ux-refinements-from-sandbox-round-3 branch from 1d44c8a to 979d585 Compare January 17, 2024 18:37

rpenido marked this pull request as ready for review January 17, 2024 18:46

rpenido changed the title ~~fix: add search highlight/expand and "no tags" message~~ fix: add search highlight/expand and "no tags" message [FC-0036] Jan 17, 2024

rpenido requested a review from yusuf-musleh January 18, 2024 13:18

yusuf-musleh approved these changes Jan 18, 2024

View reviewed changes

bradenmacdonald mentioned this pull request Jan 25, 2024

[Tagging] New "Add Tags" Widget in Tag Drawer openedx/modular-learning#188

Closed

xitij2000 approved these changes Jan 30, 2024

View reviewed changes

xitij2000 approved these changes Feb 2, 2024

View reviewed changes

rpenido changed the title ~~fix: add search highlight/expand and "no tags" message [FC-0036]~~ feat: add search highlight/expand and "no tags" message [FC-0036] Feb 2, 2024

feat: add search highlight/expand and "no tags" message

74a1684

rpenido force-pushed the rpenido/fal-3615-ux-refinements-from-sandbox-round-3 branch from bcd501f to 74a1684 Compare February 2, 2024 12:21

xitij2000 merged commit 24c48bc into openedx:master Feb 2, 2024

xitij2000 deleted the rpenido/fal-3615-ux-refinements-from-sandbox-round-3 branch February 2, 2024 15:09

rpenido mentioned this pull request Feb 6, 2024

fix: revert code due to wrong merge conflict resolution #824

Merged

yusuf-musleh mentioned this pull request Mar 4, 2024

[FC-0036] feat: New "Add Tags" widget #834

Merged


		import { useTaxonomyTagsData } from './data/apiHooks';

		const HighlightedText = ({ text, highlight }) => {

Uh oh!

Conversation

rpenido commented Jan 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Suporting Information

Testing instructions

Uh oh!

openedx-webhooks commented Jan 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

rpenido Jan 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

yusuf-musleh left a comment

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!

rpenido Jan 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

codecov Bot commented Jan 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rpenido commented Jan 18, 2024

Uh oh!

yusuf-musleh left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xitij2000 left a comment

Choose a reason for hiding this comment

Uh oh!

rpenido commented Jan 31, 2024

Uh oh!

xitij2000 commented Feb 2, 2024

Uh oh!

rpenido commented Feb 2, 2024

Uh oh!

openedx-webhooks commented Feb 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

rpenido commented Jan 16, 2024 •

edited

Loading

openedx-webhooks commented Jan 16, 2024 •

edited

Loading

rpenido Jan 16, 2024 •

edited

Loading

rpenido Jan 17, 2024 •

edited

Loading

codecov Bot commented Jan 17, 2024 •

edited

Loading

yusuf-musleh left a comment •

edited

Loading