Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: search modal refinements #959

Merged

Conversation

yusuf-musleh
Copy link
Contributor

@yusuf-musleh yusuf-musleh commented Apr 23, 2024

Description

This PR adds a few styling and UX refinements to the search modal.

Supporting information

Related Tickets:

Note:
The following points in the related github issue could not be reproduced on master:

  1. For each result, the course/library name should only appear in the breadcrumbs when “all courses” is selected. (Currently, it shows for the results in “this course” too.)
  1. If I enter a search term that yields no results, I am unable to open the “type” filter. Change it to behave like the "Tags" filter and display a message like "No types in current results" in that case.

See comment on PR for more information.

Testing instructions

  1. Run you local devstack on this branch
  2. Make sure you have the tutor-contrib-melisearch plugin installed and enabled
  3. Confirm the following:
    1. The focus state of the button outline should have a square edge to match the button shape.
      Screenshot 2024-04-28 at 1 22 02 PM

    2. The spacing between the search keywords field and "This Course" dropdown select menu has been increased to allow for few pixels between them when the button is focused.

    3. When the search modal opens, focus on the search keywords field so the user can start typing right away.

    4. Search for a keyword and confirm the following:

      1. When selecting a result, it should navigate to the page, expand the section/subsection, and scroll to it positioning it closer to the top of the page.
        Since feat: redirect to unit page if the hit or its parent is a unit #957 was merged, click on unit search result redirects directly to the unit. So in order to test this the scroll/expanding of subsection in PR for units you need to include the the unit ID in the show queryparam in the url in order to see it in action, i.e only the subsection with the Unit will expand. For example:

        http://apps.local.edly.io:2001/course-authoring/course/course-v1:SampleTaxonomyOrg1+STC1+2023_1?show=block-v1%3ASampleTaxonomyOrg1%2BSTC1%2B2023_1%2Btype%40vertical%2Bblock%40aaf8b8eb86b54281aeeab12499d2cb0b

        Note: Make sure the unit id is URL encoded.

      2. Check that if you are in a course, and the section is collapsed, and you search for a subsection in the collapsed, it expands it and scrolls to it, positioning it closer to the top


Private-ref: FAL-3718

@openedx-webhooks
Copy link

openedx-webhooks commented Apr 23, 2024

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.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Apr 23, 2024
@yusuf-musleh yusuf-musleh force-pushed the yusuf-musleh/search-modal-refinements branch 4 times, most recently from a1ab5f9 to d7d50c0 Compare April 24, 2024 14:08
Copy link

codecov bot commented Apr 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.21%. Comparing base (65f45f7) to head (ecbaba8).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #959      +/-   ##
==========================================
+ Coverage   92.20%   92.21%   +0.01%     
==========================================
  Files         703      703              
  Lines       12334    12359      +25     
  Branches     2671     2681      +10     
==========================================
+ Hits        11372    11397      +25     
  Misses        926      926              
  Partials       36       36              

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

@yusuf-musleh yusuf-musleh force-pushed the yusuf-musleh/search-modal-refinements branch from d7d50c0 to 8b23a54 Compare April 28, 2024 10:20
@yusuf-musleh yusuf-musleh marked this pull request as ready for review April 28, 2024 11:33
@yusuf-musleh yusuf-musleh requested a review from a team as a code owner April 28, 2024 11:33
@ChrisChV
Copy link
Contributor

@yusuf-musleh

Search for a keyword and confirm the following:
When selecting a result, it should navigate to the page, expand the section/subsection, and scroll to it positioning it closer to the top of the page.
Check that if you are in a course, and the section is collapsed, and you search for a subsection or unit in the collapsed, it expands it and scrolls to it, positioning it closer to the top

This expands all sections and subsections of the course, only the one that has been searched should expand

https://www.loom.com/share/0e8b880621d84ffa9de7c834155e50e0?sid=3aa2299d-6019-4f53-979a-0824061106f0

@yusuf-musleh yusuf-musleh force-pushed the yusuf-musleh/search-modal-refinements branch 2 times, most recently from 1e89c13 to 4a8c917 Compare April 30, 2024 16:33
This includes the following:

  - The target search element is aligned to the top of the page when scrolling to it
  - Makes sure the section/subsection is expanded in order to scroll to result
@yusuf-musleh yusuf-musleh force-pushed the yusuf-musleh/search-modal-refinements branch 2 times, most recently from 9c489c9 to a91b936 Compare May 1, 2024 14:02
@yusuf-musleh yusuf-musleh force-pushed the yusuf-musleh/search-modal-refinements branch from a91b936 to ecbaba8 Compare May 1, 2024 16:06
@yusuf-musleh
Copy link
Contributor Author

@ChrisChV Thanks for the review! I've updated the implementation to only expand the section/subsection that contains the search result. It's ready for another round.

However, since #957 was merged, which redirects directly to the unit when you click on it in the search results. So in order to test this PR for units you need to include the the unit ID in the show queryparam in the url in order to see it in action, i.e only the subsection with the Unit will expand. For example:

http://apps.local.edly.io:2001/course-authoring/course/course-v1:SampleTaxonomyOrg1+STC1+2023_1?show=block-v1%3ASampleTaxonomyOrg1%2BSTC1%2B2023_1%2Btype%40vertical%2Bblock%40aaf8b8eb86b54281aeeab12499d2cb0b

Note: Make sure the unit id is URL encoded.

To test the subsections, you can collapse the sections in the page, and search for it normally, then click on the subsection result, only the section containing that subsection will expand.

I will work on the other requested changes @bradenmacdonald mentioned as part of a separate PR.

Copy link
Contributor

@ChrisChV ChrisChV left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yusuf-musleh Looks good 👍

  • 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

@yusuf-musleh
Copy link
Contributor Author

@ChrisChV Thanks for the review!

@xitij2000 This PR is ready for your review.

Copy link
Contributor

@xitij2000 xitij2000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

  • I tested this: tested on devstack
  • I read through the code
  • I checked for accessibility issues
  • Includes documentation

@xitij2000 xitij2000 merged commit 6d9a8a1 into openedx:master May 3, 2024
6 checks passed
@openedx-webhooks
Copy link

@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.

@xitij2000 xitij2000 deleted the yusuf-musleh/search-modal-refinements branch May 3, 2024 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants