feat: improve search modal results [FC-0040] #946

rpenido · 2024-04-11T17:36:10Z

Description

This PR improves the SearchResult component, which shows the hits for the search feature.

More Infomation

Closes [Course Search] Search Modal: Results modular-learning#200

Testing Instructions

Setup

Run your local stack on this branch
Make sure you have meilisearch setup locally, follow the setup instructions here https://github.com/open-craft/tutor-contrib-meilisearch
Make sure you also have some sample taxonomies/tags, you can add some from here: https://github.com/open-craft/taxonomy-sample-data
Also, verify if the contentstore.new_studio_mfe.use_new_course_outline_page waffle flag is enabled.

Testing

Click on the search icon in the Studio Header
Do some sample searches and verify the results
Clicking in the result row must redirect to the course/library page of the content
Clicking in the navigate icon at the right of the row should open the content context in a new tab
NOTE If the content is visible on the page (not collapsed), the page will be scrolled and the content will be highlighted for 5 seconds. This will NOT happen if the content is inside a collapsed parent.

Private ref: FAL-3695

openedx-webhooks · 2024-04-11T17:36:14Z

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-04-11T19:27:59Z

src/course-outline/CourseOutline.scss

+
+div.row:has(> div > div.highlight) {
+  animation: 5s glow;
+}
+
+@keyframes glow {
+  0% {
+    box-shadow: 0 0 5px 5px $primary-500;
+  }
+
+  90% {
+    box-shadow: 0 0 5px 5px $primary-500;
+  }
+
+  100% {
+    box-shadow: unset;
+  }
+}


This is the animation after scrolling to a searched component. Personally, I didn't like it. 😞
I am open to suggestions! @ali-hugo

PS: The highlight only happens if the parent item is expanded and the component is on screen. We could improve it with a follow-up task.

@rpenido I agree that the scrolling is awkward.. but I can't think of a better place to send someone who's searched for a Section/Subsection.

However, if they search for a Unit, I think we should direct them to the Unit edit page instead of scrolling down the outline, because Units are very unlikely to be expanded on that page. So then it's just Sections and Subsections that use the scrolling thing, and Units and components redirect to the Unit edit page.

@ali-hugo What do you think?

The Unit redirect will be fixed in a follow-up task!

What seems to be the issue with the animation? I think one thing that can be done to make it smoother is to use a timing function.

i.e. remove the 90% step and add a timing function like:
animation-timing-function: cubic-bezier(1,0,.72,.04);

This will be a lot smoother I think since it'll follow a curve rather than a steep fall.

What seems to be the issue with the animation?

Nothing specific. I just didn't find it "wow" (I'm not sure if it was the color, the glow size..). I think it's a subjective feeling, and I'm open to options to improve it (if that's the case).

Changed the curve here: 12ba0c5

rpenido · 2024-04-11T19:34:13Z

src/search-modal/SearchResult.jsx

+    }
+
+    navigate(url);
+    closeSearch?.();


I had to pass the closeSearch function down the path SearchModal > SearchEndpointLoader > SearchUI > SearchResult to close the modal. Maybe we should add a context to handle this state?

Is that how our other modals work? If so, then yes, I agree that's worth doing here too.

We don't have this problem in the other modals because we don't have a deep inner component interacting with it.
The current approach is the simple-but-not-so-beauty way. We could hear more from cc/upstream review if we can improve it without adding complexity.

I added it to a context in my PR when I merged it with this one - see here.

rpenido · 2024-04-11T19:35:06Z

src/custom.d.ts

+declare module '*.svg' {
+  const content: string;
+  export default content;
+}


To support importing svg files as image source

pomegranited

Hi @rpenido , this is looking and working beautifully! I found a couple of minor nits and a couple of omissions from the Figma designs, noted inline.

pomegranited · 2024-04-12T01:59:24Z

src/search-modal/SearchResult.jsx

+  const { context_key: contextKey, usage_key: usageKey } = hit;
+  if (contextKey.startsWith('course-v1:')) {
+    if (newWindow) {
+      return `${getPath(getConfig().PUBLIC_PATH)}/course/${contextKey}?show=${encodeURIComponent(usageKey)}`;


getPath returns a trailing /, so this link ends up with //course/ in the path, and fails to load.

Suggested change

return `${getPath(getConfig().PUBLIC_PATH)}/course/${contextKey}?show=${encodeURIComponent(usageKey)}`;

return `${getPath(getConfig().PUBLIC_PATH)}course/${contextKey}?show=${encodeURIComponent(usageKey)}`;

Also, since these returned URLs share a common path string, can that be reflected in the code too?

And finally: Figma says that when we click on a component (text, video, etc), we should be directed to the Unit edit page with the component highlighted. instead of just trying to scroll down to the Unit in the outline.

(I also think searching for a Unit should take us to the Unit edit page too, but that's up to UI/UX, see above :)

Also, since these returned URLs share a common path string, can that be reflected in the code too?

Sure! Done here: 53d70e3

And finally: Figma says that when we click on a component (text, video, etc), we should be directed to the Unit edit page with the component highlighted. instead of just trying to scroll down to the Unit in the outline.

Ack! We will need a follow-up task to that because right now, we don't have the unit info in the component result.

pomegranited · 2024-04-12T02:21:46Z

src/search-modal/SearchResult.jsx

+    return `/course/${contextKey}?show=${encodeURIComponent(usageKey)}`;
+  }
+  if (usageKey.includes('lb:')) {
+    return `${libraryAuthoringMfeUrl}library/${contextKey}`;


Not everyone has the library authoring MFE enabled.. and I don't think it'll be ready for Redwood either.. Is there some MFE way of detecting this and getting the right base URL to use?

Yes!
I made a refactor here: 6cb2d96
If we don't have the redirect URL the hit will be shown as disabled.

pomegranited · 2024-04-12T02:33:36Z

src/search-modal/SearchResult.jsx

+    }
+    return `/course/${contextKey}?show=${encodeURIComponent(usageKey)}`;
+  }
+  if (usageKey.includes('lb:')) {


nit -- since you used startsWith above:

Suggested change

if (usageKey.includes('lb:')) {

if (usageKey.startsWith('lb:')) {

Someday soon we're going to want MFE libraries for opaque keys.. but that's out of scope here :)

Done: 53d70e3

pomegranited · 2024-04-12T02:37:57Z

src/search-modal/SearchResult.jsx

+    }
+
+    navigate(url);
+    closeSearch?.();


Is that how our other modals work? If so, then yes, I agree that's worth doing here too.

pomegranited · 2024-04-12T02:48:11Z

src/search-modal/SearchResult.jsx

+        gap={2}
+      >
+        <div className="align-top">
+          <Icon className="align-top" src={ItemIcon[hit.block_type] || Article} />


The icons in Figma look to be a lighter grey, and maybe a bit more padding between the icon and the text?

Figma:

MFE:

Done: f1b3b62

pomegranited · 2024-04-12T02:57:25Z

src/search-modal/SearchResult.jsx

+            <Snippet attribute="content.capa_content" hit={hit} highlightedTagName="b" />
+          </div>
+          <div className="text-muted x-small">
+            <CustomHighlight attribute="breadcrumbsNames" separator=" / " hit={hit} />


The figma designs omit the Course Title from the breadcrumbs when searching just "This course".

The Course Title should only be shown if we're searching "All courses".

Done: c152a42

pomegranited · 2024-04-12T03:07:02Z

src/course-outline/CourseOutline.scss

+
+div.row:has(> div > div.highlight) {
+  animation: 5s glow;
+}
+
+@keyframes glow {
+  0% {
+    box-shadow: 0 0 5px 5px $primary-500;
+  }
+
+  90% {
+    box-shadow: 0 0 5px 5px $primary-500;
+  }
+
+  100% {
+    box-shadow: unset;
+  }
+}


@rpenido I agree that the scrolling is awkward.. but I can't think of a better place to send someone who's searched for a Section/Subsection.

However, if they search for a Unit, I think we should direct them to the Unit edit page instead of scrolling down the outline, because Units are very unlikely to be expanded on that page. So then it's just Sections and Subsections that use the scrolling thing, and Units and components redirect to the Unit edit page.

@ali-hugo What do you think?

Co-authored-by: Jillian <[email protected]>

bradenmacdonald · 2024-04-14T02:11:41Z

@rpenido I noticed a couple warnings in the console:

Failed prop type: The prop alt is marked as required in ForwardRef(_c), but its value is undefined. on <IconButton> used in <SearchResult>
validateDOMNesting(...): <button> cannot appear as a descendant of <button>. - seems to be this problem: Search Result contains a <Button> which contains a <Stack> which contains an <IconButton>. You can't do that - change the outer one to a <div>. If it needs to be a button give it role=button or use <Button as={div}> but it's probably better not to be a button if it contains a button.

pomegranited

Thanks for addressing my comments @rpenido 👍

I've left a comment about missing tests, but I'm ok with them being covered by your follow-up task.

I tested this on my tutor dev stack using the PR test instructions
I read through the code
I checked for accessibility issues by navigating with my keyboard.
Includes documentation
User-facing strings are extracted for translation

pomegranited · 2024-04-16T05:23:09Z

src/search-modal/SearchModal.test.jsx

@@ -59,7 +64,7 @@ describe('<SearchModal />', () => {
      apiKey: 'test-api-key',
    });
    const { findByText } = render(<RootWrapper />);
-    expect(await findByText('Enter a keyword or select a filter to begin searching.')).toBeInTheDocument();
+    expect(await findByText('Start searching to find content')).toBeInTheDocument();


Can you add more tests to cover the added functionality for this SearchModal?

More tests: 879a395, 66612bc, 58a5c8c

xitij2000

I think this is good to go on my side. Just very few small issues that I think would be nice if addressed.

xitij2000 · 2024-04-16T12:59:49Z

src/search-modal/SearchModal.scss

+  .fs-large {
+    font-size: $font-size-base * 1.25;
  }


You can use the class lead instead of using this. ref

Nice! Thank you!
Done: 78e0a81

xitij2000 · 2024-04-16T13:01:15Z

src/search-modal/EmptyStates.jsx

+const EmptySearch = () => (
+  <Stack className="d-flex mt-6 align-items-center">
+    <p className="fs-large"> <FormattedMessage {...messages.emptySearchTitle} /> </p>
+    <p className="small text-muted"> <FormattedMessage {...messages.emptySearchSubtitle} /> </p>
+    <img src={EmptySearchImage} alt="" />
+  </Stack>
+);
+
+const NoResults = () => (
+  <Stack className="d-flex mt-6 align-items-center">
+    <p className="fs-large"> <FormattedMessage {...messages.noResultsTitle} /> </p>
+    <p className="small text-muted"> <FormattedMessage {...messages.noResultsSubtitle} /> </p>
+    <img src={NoResultImage} alt="" />
+  </Stack>
+);
+


I think these should be deduplicated since they have the same structure, and will likely need to always have the same structure.

Done: 78e0a81

xitij2000 · 2024-04-16T13:12:49Z

src/course-outline/CourseOutline.scss

+
+div.row:has(> div > div.highlight) {
+  animation: 5s glow;
+}
+
+@keyframes glow {
+  0% {
+    box-shadow: 0 0 5px 5px $primary-500;
+  }
+
+  90% {
+    box-shadow: 0 0 5px 5px $primary-500;
+  }
+
+  100% {
+    box-shadow: unset;
+  }
+}


What seems to be the issue with the animation? I think one thing that can be done to make it smoother is to use a timing function.

i.e. remove the 90% step and add a timing function like:
animation-timing-function: cubic-bezier(1,0,.72,.04);

This will be a lot smoother I think since it'll follow a curve rather than a steep fall.

rpenido · 2024-04-16T13:48:12Z

@rpenido I noticed a couple warnings in the console:

1. Failed prop type: The prop `alt` is marked as required in `ForwardRef(_c)`, but its value is `undefined`. on `<IconButton>` used in `<SearchResult>`

2. `validateDOMNesting(...): <button> cannot appear as a descendant of <button>.` - seems to be this problem: Search Result contains a `<Button>` which contains a `<Stack>` which contains an `<IconButton>`. You can't do that - change the outer one to a `<div>`. If it needs to be a button give it `role=button` or use `<Button as={div}>` but it's probably better not to be a button if it contains a button.

Thank you for catching that @bradenmacdonald!
Fixed here: 8e468ef
It needed a bit more of a boilerplate to make it keyboard-friendly.

src/search-modal/SearchResult.jsx

Co-authored-by: Braden MacDonald <[email protected]>

codecov · 2024-04-16T19:58:07Z

Codecov Report

Attention: Patch coverage is 96.72131% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 92.09%. Comparing base (b119671) to head (7d037f2).

Files	Patch %	Lines
src/search-modal/SearchResult.jsx	93.93%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #946      +/-   ##
==========================================
+ Coverage   92.06%   92.09%   +0.02%     
==========================================
  Files         634      634              
  Lines       11029    11080      +51     
  Branches     2367     2387      +20     
==========================================
+ Hits        10154    10204      +50     
- Misses        846      847       +1     
  Partials       29       29

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

…rmat-results

openedx-webhooks · 2024-04-17T08:13:59Z

@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 Apr 11, 2024

rpenido force-pushed the rpenido/fal-3695-implement-search-modal-format-results branch 2 times, most recently from 5fc9b54 to 21e7cab Compare April 11, 2024 17:59

rpenido mentioned this pull request Apr 11, 2024

TEMP open-craft/frontend-app-authoring#33

Closed

rpenido force-pushed the rpenido/fal-3695-implement-search-modal-format-results branch 3 times, most recently from bb31469 to 6230900 Compare April 11, 2024 18:42

feat: improve search modal results

14ded36

rpenido force-pushed the rpenido/fal-3695-implement-search-modal-format-results branch from 6230900 to 14ded36 Compare April 11, 2024 19:15

rpenido commented Apr 11, 2024

View reviewed changes

feat: showMoreResults and SearchResult adjustments

cd66611

rpenido marked this pull request as ready for review April 11, 2024 21:59

rpenido requested a review from a team as a code owner April 11, 2024 21:59

rpenido requested a review from pomegranited April 11, 2024 22:44

pomegranited suggested changes Apr 12, 2024

View reviewed changes

rpenido and others added 2 commits April 12, 2024 14:22

fix: remove trailling / after getPath

f4cf7fd

Co-authored-by: Jillian <[email protected]>

refactor: use same path sufix

53d70e3

rpenido force-pushed the rpenido/fal-3695-implement-search-modal-format-results branch from b71b0b5 to 53d70e3 Compare April 12, 2024 17:44

refactor: url redirect for libraries

6cb2d96

rpenido force-pushed the rpenido/fal-3695-implement-search-modal-format-results branch from f98778b to 6cb2d96 Compare April 12, 2024 18:14

rpenido added 3 commits April 12, 2024 15:27

fix: result style adjustments

f1b3b62

fix: remove course from breadcrumbs if searchThisCourse

c152a42

fix: return if no redirectUrl

afa8fdd

rpenido requested a review from pomegranited April 12, 2024 19:01

pomegranited approved these changes Apr 16, 2024

View reviewed changes

xitij2000 approved these changes Apr 16, 2024

View reviewed changes

rpenido force-pushed the rpenido/fal-3695-implement-search-modal-format-results branch from 838c55e to 4860614 Compare April 16, 2024 13:39

fix: console warnings

8e468ef

rpenido force-pushed the rpenido/fal-3695-implement-search-modal-format-results branch from 4860614 to 8e468ef Compare April 16, 2024 13:59

bradenmacdonald reviewed Apr 16, 2024

View reviewed changes

src/search-modal/SearchResult.jsx Outdated Show resolved Hide resolved

refactor: change animation curve

079e52e

rpenido force-pushed the rpenido/fal-3695-implement-search-modal-format-results branch from 12ba0c5 to 079e52e Compare April 16, 2024 19:32

rpenido and others added 2 commits April 16, 2024 16:40

refactor: deduplicate component and remove fs-large style

78e0a81

fix: improve type guard and remove ts-ignore

8c75c8d

Co-authored-by: Braden MacDonald <[email protected]>

rpenido added 2 commits April 16, 2024 17:14

Merge branch 'master' into rpenido/fal-3695-implement-search-modal-fo…

68da2b9

…rmat-results

test: add empty result test

879a395

rpenido force-pushed the rpenido/fal-3695-implement-search-modal-format-results branch from 3e52a20 to 879a395 Compare April 16, 2024 21:05

test: test result and new window click

66612bc

rpenido force-pushed the rpenido/fal-3695-implement-search-modal-format-results branch from 17e4012 to 66612bc Compare April 16, 2024 21:52

rpenido added 2 commits April 16, 2024 19:22

test: add library and fix tests

58a5c8c

test: remove coverage from library cases

7d037f2

rpenido force-pushed the rpenido/fal-3695-implement-search-modal-format-results branch from 8cd53fe to 7d037f2 Compare April 17, 2024 04:59

xitij2000 merged commit 612d1d8 into openedx:master Apr 17, 2024
6 checks passed

rpenido deleted the rpenido/fal-3695-implement-search-modal-format-results branch April 17, 2024 12:20

	return `${getPath(getConfig().PUBLIC_PATH)}/course/${contextKey}?show=${encodeURIComponent(usageKey)}`;
	return `${getPath(getConfig().PUBLIC_PATH)}course/${contextKey}?show=${encodeURIComponent(usageKey)}`;

	if (usageKey.includes('lb:')) {
	if (usageKey.startsWith('lb:')) {

feat: improve search modal results [FC-0040] #946

feat: improve search modal results [FC-0040] #946

Conversation

rpenido commented Apr 11, 2024 • edited Loading

Description

More Infomation

Testing Instructions

Setup

Testing

openedx-webhooks commented Apr 11, 2024 • edited Loading

rpenido Apr 11, 2024 • 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

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

pomegranited 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

rpenido Apr 12, 2024 • 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

Choose a reason for hiding this comment

rpenido Apr 12, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bradenmacdonald commented Apr 14, 2024

pomegranited left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

rpenido commented Apr 16, 2024 • edited Loading

codecov bot commented Apr 16, 2024 • edited Loading

Codecov Report

openedx-webhooks commented Apr 17, 2024

rpenido commented Apr 11, 2024 •

edited

Loading

openedx-webhooks commented Apr 11, 2024 •

edited

Loading

rpenido Apr 11, 2024 •

edited

Loading

rpenido Apr 12, 2024 •

edited

Loading

rpenido Apr 12, 2024 •

edited

Loading

rpenido commented Apr 16, 2024 •

edited

Loading

codecov bot commented Apr 16, 2024 •

edited

Loading