feat: redirect to unit page if the hit or its parent is a unit #957

rpenido · 2024-04-19T20:31:08Z

Description

This PR adds the feature to redirect from a search result to a Unit, in case the hit parent is a Unit, or the hit is a unit itself.

More Information:

Closes:

[Course Search] Allow redirect to the unit page from the search results modular-learning#210

Depends on:

Testing Instructions

Go to the Course Authoring MFE, e.g. http://apps.local.edly.io:2001/course-authoring/home
Optionally, browse to a specific course.
Click on the search icon in the header
Search for a Unit and/or a component inside a unit and click on it
You should be redirected to the unit page and the selected component should be highlighted (if any).

Private ref: FAL-3709

openedx-webhooks · 2024-04-19T20:31:13Z

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.

codecov · 2024-04-23T19:34:28Z

Codecov Report

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

Project coverage is 92.09%. Comparing base (5686dee) to head (0493375).
Report is 3 commits behind head on master.

Files	Patch %	Lines
src/course-unit/hooks.jsx	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #957      +/-   ##
==========================================
+ Coverage   92.02%   92.09%   +0.06%     
==========================================
  Files         686      685       -1     
  Lines       11955    12090     +135     
  Branches     2598     2637      +39     
==========================================
+ Hits        11002    11134     +132     
- Misses        917      920       +3     
  Partials       36       36

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

ChrisChV · 2024-04-24T15:09:13Z

@rpenido

You should be redirected to the unit page and the selected component should be highlighted (if any).

When I search for a component it does not redirect me to the unit page:

https://www.loom.com/share/1c5633da07804908980a158e3dfc0289?sid=638b4634-5b63-461c-9206-5548d5ccf2e3

rpenido · 2024-04-24T16:42:43Z

@rpenido

You should be redirected to the unit page and the selected component should be highlighted (if any).

When I search for a component it does not redirect me to the unit page:

https://www.loom.com/share/1c5633da07804908980a158e3dfc0289?sid=638b4634-5b63-461c-9206-5548d5ccf2e3

Hi @ChrisChV! Thank you for your input!
It seems that your breadcrumbs are broken. Could you check if you are using openedx/edx-platform#34535 and please run tutor dev run cms ./manage.py cms reindex_studio --experimental again?

…t-page-from-search-results

ChrisChV

@rpenido Thanks! It works 👍

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.

xitij2000

I think this is looking good.

I just have one minor nit about reusing styles. The other point is entirely optional since I don't see anything wrong with the current implementation.

xitij2000 · 2024-04-26T08:58:49Z

src/course-unit/CourseUnit.scss

+@keyframes glow {
+  0% {
+    box-shadow: 0 0 5px 5px $primary-500;
+  }
+
+  100% {
+    box-shadow: unset;
+  }
+}


This exact same animation already exists in CourseOutline.scss, can you boost it to a more common location and reuse the code instead?

Thank you for the suggestion @xitij2000! I moved it here: 0493375

xitij2000 · 2024-04-26T09:12:16Z

src/course-unit/course-xblock/CourseXBlock.jsx

+    if (courseXBlockElementRef.current && (shouldScroll || isScrolledToElement)) {
      scrollToElement(courseXBlockElementRef.current);
    }
-  }, []);
+  }, [isScrolledToElement]);


FYI, there is another way to do this without using react state at all.

Give the element a unique ID e.g. xblock-id-abcde

Set the url hash to the above ID, i.e. "...some/url/page#xblock-id-abcde"

Use the ":target" pseudo-class to apply the glow style.

If the URL fragment matches an ID the browser should automatically scroll to it, and it applies the :target pseudo-class ref.

This means by using this mechanism you can avoid any code for scrolling, and applying a class. Also changing this value doesn't cause the page to refresh since it's client-side only.

The only disadvantage is that this is never passed to the server. What that means is that the server can't perform any action based on this value. Here since there is no server this is a moot point, but worth mentioning anyway.

Awesome @xitij2000! Thank you for this.

Using targets here would be a better approach, but I think refactoring it (we already had some scrolling function feature) will put us out of schedule/scope.

But I think it is worth refactoring it next time we hit this feature.

Certainly, that makes sense, and keeping this part of the JavaScript code also opens up other possibilities. No need for a refactoring already.

rpenido · 2024-04-29T13:05:40Z

Hi @xitij2000 ! Could you do another pass and merge if everything is fine?

openedx-webhooks · 2024-04-29T14:20:15Z

@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 19, 2024

rpenido force-pushed the rpenido/fal-3709-allow-redirect-to-the-unit-page-from-search-results branch 5 times, most recently from 33b131e to 9798dc3 Compare April 22, 2024 14:58

rpenido mentioned this pull request Apr 23, 2024

feat: redirect to unit page if the hit or its parent is a unit (TEMP) open-craft/frontend-app-authoring#35

Closed

rpenido force-pushed the rpenido/fal-3709-allow-redirect-to-the-unit-page-from-search-results branch 5 times, most recently from fc5df69 to 0d38d98 Compare April 24, 2024 12:39

feat: redirect to unit page if the hit or its parent is a unit

9901d9b

rpenido force-pushed the rpenido/fal-3709-allow-redirect-to-the-unit-page-from-search-results branch from 0d38d98 to 9901d9b Compare April 24, 2024 12:48

rpenido added 2 commits April 24, 2024 10:08

test: add comments

1bbca0a

test: improve test coverage

949b3c6

rpenido force-pushed the rpenido/fal-3709-allow-redirect-to-the-unit-page-from-search-results branch from 64c630e to 949b3c6 Compare April 24, 2024 14:36

rpenido force-pushed the rpenido/fal-3709-allow-redirect-to-the-unit-page-from-search-results branch 2 times, most recently from 0e4d37f to 1954667 Compare April 24, 2024 18:01

test: add lib content tests

78dc0c4

rpenido force-pushed the rpenido/fal-3709-allow-redirect-to-the-unit-page-from-search-results branch from 1954667 to 78dc0c4 Compare April 24, 2024 18:04

rpenido and others added 3 commits April 24, 2024 15:19

test: more lib content tests

f873a98

Merge branch 'master' into rpenido/fal-3709-allow-redirect-to-the-uni…

f04e43d

…t-page-from-search-results

fix: merge error

a13ccb5

ChrisChV approved these changes Apr 25, 2024

View reviewed changes

rpenido marked this pull request as ready for review April 25, 2024 19:23

rpenido requested a review from a team as a code owner April 25, 2024 19:23

rpenido requested a review from xitij2000 April 25, 2024 19:23

xitij2000 approved these changes Apr 26, 2024

View reviewed changes

refactor: moving css style

0493375

rpenido requested a review from xitij2000 April 29, 2024 13:05

xitij2000 merged commit e24fb78 into openedx:master Apr 29, 2024
6 checks passed

xitij2000 deleted the rpenido/fal-3709-allow-redirect-to-the-unit-page-from-search-results branch April 29, 2024 14:20

yusuf-musleh mentioned this pull request May 1, 2024

feat: search modal refinements #959

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: redirect to unit page if the hit or its parent is a unit #957

feat: redirect to unit page if the hit or its parent is a unit #957

rpenido commented Apr 19, 2024 •

edited

Loading

openedx-webhooks commented Apr 19, 2024 •

edited

Loading

codecov bot commented Apr 23, 2024 •

edited

Loading

ChrisChV commented Apr 24, 2024

rpenido commented Apr 24, 2024

ChrisChV left a comment

xitij2000 left a comment

xitij2000 Apr 26, 2024

rpenido Apr 29, 2024

xitij2000 Apr 26, 2024

rpenido Apr 29, 2024

xitij2000 Apr 29, 2024

rpenido commented Apr 29, 2024

openedx-webhooks commented Apr 29, 2024

feat: redirect to unit page if the hit or its parent is a unit #957

feat: redirect to unit page if the hit or its parent is a unit #957

Conversation

rpenido commented Apr 19, 2024 • edited Loading

Description

More Information:

Testing Instructions

openedx-webhooks commented Apr 19, 2024 • edited Loading

codecov bot commented Apr 23, 2024 • edited Loading

Codecov Report

ChrisChV commented Apr 24, 2024

rpenido commented Apr 24, 2024

ChrisChV left a comment

Choose a reason for hiding this comment

xitij2000 left a comment

Choose a reason for hiding this comment

xitij2000 Apr 26, 2024

Choose a reason for hiding this comment

rpenido Apr 29, 2024

Choose a reason for hiding this comment

xitij2000 Apr 26, 2024

Choose a reason for hiding this comment

rpenido Apr 29, 2024

Choose a reason for hiding this comment

xitij2000 Apr 29, 2024

Choose a reason for hiding this comment

rpenido commented Apr 29, 2024

openedx-webhooks commented Apr 29, 2024

rpenido commented Apr 19, 2024 •

edited

Loading

openedx-webhooks commented Apr 19, 2024 •

edited

Loading

codecov bot commented Apr 23, 2024 •

edited

Loading