UI fixes for Sort Library Component [FC-0059] #1222
Conversation
When search keyword(s) are entered, use "Most Relevant" as default. Also * Hides "Most Relevant" option if no keyword is entered. * Re-orders the sort menu options * Ensures the default sort option is not stored in the query string * Shows the selected sort option on the drop-down toggle button * Shows "Sort By" as a header inside the drop-down menu
Instead, reset sort option to default if the selected sort option is re-clicked.
* Updates sort components tests to verify changes. * "Recently Modified" now appears as the default sort option in the toggle button, and as a header on the Home page, so we need to count them both. * Fewer queries were executed with the updated code.
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @pomegranited! What's next?Please work through the following steps to get your changes ready for engineering review: 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. 🔘 Let us know that your PR is ready for review:Who will review my changes?This repository is currently maintained by Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:
When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
pomegranited
added
create-sandbox
openedx-webhooks
added
the
open-source-contribution
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1222 +/- ##
==========================================
- Coverage 93.03% 93.01% -0.02%
==========================================
Files 756 756
Lines 13716 13722 +6
Branches 2969 2966 -3
==========================================
+ Hits 12760 12764 +4
- Misses 920 923 +3
+ Partials 36 35 -1 ☔ View full report in Codecov by Sentry. |
|
Sandbox deployment successful 🚀 |
There was a problem hiding this comment.
👍 @pomegranited Works really well, great work! Just left a small comment about the new styles.
- I tested this: Followed the testing instructions
- I read through the code
- I checked for accessibility issues
- Includes documentation
| @@ -62,13 +86,18 @@ export const SearchSortWidget: React.FC<Record<never, never>> = () => { | |||
| size="sm" | |||
| > | |||
| <Icon src={SwapVert} className="d-inline" /> | |||
| {searchSortLabel} | |||
| <div className="search-sort-toggle-label">{toggleLabel}</div> | |||
There was a problem hiding this comment.
| <div className="search-sort-toggle-label">{toggleLabel}</div> | |
| <div className="py-0 px-1">{toggleLabel}</div> |
You can achieve the same styling by using the bootstrap classes without the need to add the new CSS file SearchSortWidget.scss
There was a problem hiding this comment.
Ach, thank you @yusuf-musleh ! Much cleaner this way.
|
Sandbox deployment failed 💥 |
|
Sandbox deployment failed 💥 |
|
Sandbox deployment failed 💥 |
| // Call 1: To fetch searchable/filterable/sortable library data | ||
| // Call 2: To fetch the recently modified components only | ||
| await waitFor(() => { expect(fetchMock).toHaveFetchedTimes(2, searchEndpoint, 'post'); }); | ||
| await waitFor(() => { expect(fetchMock).toHaveFetchedTimes(1, searchEndpoint, 'post'); }); |
There was a problem hiding this comment.
@pomegranited One question: what changes here to decrease the number of calls of the searchEndpoint?
There was a problem hiding this comment.
Honestly, I wish I knew for sure.. Maybe it's something to do with the default sort option not being empty anymore (since it's created:desc now), and maybe the search manager was setting it twice before for some reason?
But the premise in those deleted comments is not correct -- yes, we make 2 queries, but they're done as part of a single hit to the /multi-search endpoint. So it should have been 1 hit all along.
That's my theory, anyway.
|
Sandbox deployment failed 💥 |
|
@pomegranited 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Description
Addresses issues raised during acceptance testing for #1038, specifically:
Hide this sort option if no keyword is entered.
Dropdown.Header.Instead, remove a sort filter by clicking on the current "sort by" selection.
For example, clicking on Sort > "Title, A,Z" will sort by that, clicking it again will remove that Sort and take the user back to default.
Screenshots
See #1147 for previous screenshots.
Default sort is "Recently Modified" (when no search keywords):
Default sort is "Most Relevant" (when search keywords provided):
Supporting information
See discussion from comment.
Private-ref: FAL-3811
Testing instructions
To test this change:
For any non-default sort option, there should be a
?sort=...entry added to the query string.