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

Sort Library Components #1038

Closed
bradenmacdonald opened this issue May 24, 2024 · 26 comments
Closed

Sort Library Components #1038

bradenmacdonald opened this issue May 24, 2024 · 26 comments

Comments

@bradenmacdonald
Copy link
Contributor

bradenmacdonald commented May 24, 2024

✅ In Scope:

  1. Update the backend so that the search index stores the creation, modification, and publication date for each library component.
  2. Implement the sort widget shown in the UI in Figma:
    sort-example
    • For now, it only needs to work on the "Components" tab.
    • It needs to support these options:
      a. Title, A-Z
      b. Title, Z-A
      c. newest
      d. oldest
      e. recently published
      f. recently modified
  3. The sort widget should be visible on all tabs, and it should "remember" the current sort option when changing tabs.
  4. The sort option should be reflected in the URL, e.g. ?sort=created but should not create a new history entry, so pressing BACK in the browser goes back to the previous page, not the previous sort option.
@bradenmacdonald bradenmacdonald added this to the Lib Relaunch 1: Library Home Page milestone May 24, 2024
@ChrisChV
Copy link
Contributor

ChrisChV commented Jun 5, 2024

For now, it only needs to work on the "Components" tab.

@bradenmacdonald As I see it, this functionality is going to be added in Home tab in #1039, right? If so I think #1048 and #1049 will be done in Home in #1039 too.

@bradenmacdonald
Copy link
Contributor Author

@ChrisChV

As I see it, this functionality is going to be added in Home tab in #1039, right?
Yes.

If so I think #1048 and #1049 will be done in Home in #1039 too.

Sure, that makes sense. Though we may implement it for all tabs at once, rather than implementing each tab separately. After all the tabs are very similar.

@pomegranited
Copy link
Contributor

Hi @jmakowski1123 CC @bradenmacdonald

e. recently published

Should selecting this option also filter out any components which have not been published?
If we don't filter, un-published components will appear at the bottom of the list, but this might be confusing if no components have been published yet.

@pomegranited
Copy link
Contributor

Hi @jmakowski1123 CC @bradenmacdonald

Since we're also doing keyword search, I kept the default sorting as keyword search relevance. To make this clear to users (and make my code simpler), I chose to show this option to users at the top of the list of sort options:

image

  • Is this ok to do?
  • Is "Most Relevant" the label to use?

@jmakowski1123
Copy link

Hi @jmakowski1123 CC @bradenmacdonald

e. recently published

Should selecting this option also filter out any components which have not been published? If we don't filter, un-published components will appear at the bottom of the list, but this might be confusing if no components have been published yet.

Correct, the Recently Published filter should remove any content that is not yet published.

@jmakowski1123
Copy link

Hi @jmakowski1123 CC @bradenmacdonald

Since we're also doing keyword search, I kept the default sorting as keyword search relevance. To make this clear to users (and make my code simpler), I chose to show this option to users at the top of the list of sort options:

image

  • Is this ok to do?
  • Is "Most Relevant" the label to use?

I think it seems redundant to indicate to users that the default sorting is by most relevant - I think that will be assumed. And if a user wants to override that, they can choose which sort they want, which I think is also intuitive. I would suggest to not include the "Most relevant" as a sort option, and rather assume that users will understand that this is the default behavior.

@pomegranited
Copy link
Contributor

pomegranited commented Jul 22, 2024

@jmakowski1123

And if a user wants to override that, they can choose which sort they want, which I think is also intuitive. I would suggest to not include the "Most relevant" as a sort option, and rather assume that users will understand that this is the default behavior.

I agree -- but we need a way to clear the selected sort option too, which means we need a selectable option in the drop-down. I can use "Sort" instead of "Most relevant"...

EDIT -- Nevermind, I rebased and found the "clear filters" action, so can use that instead of showing a menu option.

@pomegranited
Copy link
Contributor

@jmakowski1123 If there are no components published, then we see the "You have not added any content to this library" message when sort/filtering by "Recently Published".

If filters are applied, we can show another message and/or link to the "clear filters" action. How should that message read?

@jmakowski1123
Copy link

@pomegranited I think it would be clearer to users if the messages reflect the specifics of the sort status. So in the case of having a filter applied for "Recently Published" but no components published, then the message could say "You have not added any recently published content."

@pomegranited
Copy link
Contributor

@jmakowski1123 Ach, there's another edge case -- if search keywords and/or tag or block type filters are applied + sorting by "recently published", then we'd need a more complex message.. So I chose to do this instead:

image

Is that clear enough?

@pomegranited
Copy link
Contributor

pomegranited commented Jul 30, 2024

Hi @jmakowski1123 -- I've deployed these changes to the sandbox so you can see them in action: https://app.tagging-preview.staging.do.opencraft.hosting/course-authoring/libraries

Couple of things to note:

  1. There's a bug in the search index field "Recently Published" -- we're working on this here: fix: content search last_published date [FC-0059] edx-platform#35195
  2. It's odd the way the button just says "Sort" at first, but once you choose a sort, it's not immediately clear how to revert to the default sort, or see what the default sort is.
    This was the requested behavior, but I'm interested in how it feels when you use it.
  3. The "Sort" button label is inconsistent with the filters, which say "Type: X". If it were consistent, the sort button would say "Sort: Title A-Z" etc.
    Let me know if you want this changed?
  4. The selected sort is now preserved in the query string, but the keywords and filters are not.
    We may want to make that consistent down the road and store everything in the query string -- this is easy to do now that we've done it once.

@pomegranited
Copy link
Contributor

@jmakowski1123

  1. There's a bug in the search index field "Recently Published" -- we're working on this here: fix: content search last_published date [FC-0059] edx-platform#35195

FYI this bug has been fixed and deployed to the sandbox.

I created lib:OpenCraft:sortLib to illustrate, with these components:

  • "Problem (published)" -- created 1st, but modified 2nd.
  • "Text (published)" -- created 2nd but modified 1st. Published with "Problem (published)".
  • "Text (unpublished)" -- created 3rd and modified 3rd
  • "Blank Problem" -- created 4th, unmodified

@lizc577
Copy link

lizc577 commented Aug 7, 2024

Let's 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. @jmakowski1123 any thoughts?

@pomegranited
Copy link
Contributor

pomegranited commented Aug 12, 2024

@lizc577

Let's 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.

That's a great idea, and easy to implement.

Should "clear filters" continue to reset Sorting to the default (sort by relevance)?

What do you think, @jmakowski1123 @sdaitzman @marcotuts?

@sdaitzman
Copy link

Hi @pomegranited, we discussed this some more with Jenna and decided to clarify the sort dropdown's design. Here's the updated version from Figma (rolling it out across the rest of the designs soon):

image

Since the content is/should be always sorted somehow, we'd like to show the current sort in the dropdown instead of allowing users to "clear sort." The "Sort By" title within the SelectMenu helps clarify its purpose without relying only on the icon alone, which is important for accessibility.

We're thinking of filter / sort as separate concepts in this interface, and Recently Modified will be the default sort to surface blocks with recent changes.

@pomegranited
Copy link
Contributor

Thanks for this updated design @sdaitzman !

To be clear, are you asking for the following changes to the components sort UI?

  • Use "Recently Modified" as the default sort option.
  • Remove the check_icon icon from the selected sort option in the dropdown list, and bold the selected option instead.
  • Change the "Sort by" dropdown title to match the selected sort option (so we never see "Sort by" again).
  • Replace the old_sort_icon icon used in the dropdown title with new_sort_icon.
  • Don't change the sort option when "clear filters" is clicked.

My only concern here is that this means we lose the current (implicit) default sort option, which is to sort by "relevance" using the keyword(s) entered. Could "Relevance" or "Keyword Relevance" be added to this list somewhere, even if it is not the default option?

CC @jmakowski1123 @lizc577

@jmakowski1123
Copy link

jmakowski1123 commented Aug 20, 2024

@pomegranited - Curious for your thoughts from a feasibility standpoint. From a product pov, I see two options here:

  1. Relevance could be added to the sort list, and the spec is that it would only appear as an option in the sort list when a keyword search has been applied, so that users can always return back to a sort results by relevance if they apply another sort.
  2. We batch relevant results implicitly, say surface the top 50 or 100 most relevant, and when sorts are applied to results, then the sorts apply to that batch first, with a long-tail of results after that. This is how we maintained relevance and sorting capabilities in the library database world.

Is one more feasible or much less effort than the other? Or do you see other options that I'm not seeing?

@sdaitzman
Copy link

sdaitzman commented Aug 20, 2024

@pomegranited thanks for confirming the UI details; some of those changes were intentional, and some came from Paragon component defaults. Sorry for creating any confusion with that mockup. I hope this helps clarify the UI details:

  • Yes, "Recently Modified" should be the default sort option when there is no active keyword search query
  • The check icon / bold selection switch was not a deliberate/priority change, either approach is reasonable
  • The "Sort by" dropdown title should match the selected sort option. A small, non-selectable "Sort by" title should appear within the dropdown
  • The sort icon should remain the same:
    image
  • Clearing filters should not change the sort option

When there is an active keyword search, if we have a "most relevant" heuristic (e.g. weighting by keyword match / fuzzy search distance / matched fields) I think we should default to that option, and show it within the sort dropdown. This is partly a product/scope question that depends on which search heuristics exist, so I'll defer to @jmakowski1123 and those two options

Figma 2024-08-20 14 23 10

@pomegranited
Copy link
Contributor

Thank you for those clarifications, @sdaitzman 💯

  • Yes, "Recently Modified" should be the default sort option when there is no active keyword search query
  • When there is an active keyword search, if we have a "most relevant" heuristic (e.g. weighting by keyword match / fuzzy search distance / matched fields) I think we should default to that option, and show it within the sort dropdown. This is partly a product/scope question that depends on which search heuristics exist, so I'll defer to @jmakowski1123 and those two options

I think it may be confusing to the user to have the default sort changing out from under them. It also adds complexity to the code, but if that's what is needed, we can do it. Happy to defer to your expertise.

@pomegranited
Copy link
Contributor

pomegranited commented Aug 21, 2024

@jmakowski1123 Oh sorry, I missed your comment before I replied above.

  1. Relevance could be added to the sort list, and the spec is that it would only appear as an option in the sort list when a keyword search has been applied, so that users can always return back to a sort results by relevance if they apply another sort.

I think this will be the most straightforward option to implement.
Complex search filters are very possible with Meilisearch, but it gets harder to predict as a user.

I'll give this approach a try and put it up a sandbox for testing.

@pomegranited
Copy link
Contributor

Hi @jmakowski1123 @sdaitzman @lizc577 @marcotuts , there's a partially-working sandbox up that demonstrates the sort widget changes requested here. See #1222 for details and screenshots. Unfortunately the sandbox isn't letting me create library components, but you can at least see the updated default sort options, and how they change when you enter a keyword into the search box.

🎓 LMS
📝 Studio
MFE: https://app.pr-1222-11bdb6.sandboxes.opencraft.hosting/course-authoring/library/lib:OpenCraft:libA

openedx@example.com / openedx

If this looks ok enough, I'll get #1222 merged and deployed to the tagging-preview sandbox where it can be tested for reals.

@sdaitzman
Copy link

Thanks @pomegranited. One minor nitpick is that in its :active state, the menu item's text should change to a light/white color to maintain contrast. Otherwise, looks great to me!

Screenshot 2024-08-22 at 3 11 53 PM Screenshot 2024-08-22 at 3 14 25 PM

@pomegranited
Copy link
Contributor

@sdaitzman Thank you for your review!

One minor nitpick is that in its :active state, the menu item's text should change to a light/white color to maintain contrast.

Interesting.. I'm seeing that issue on the sandbox, but not in my dev environment, so I think that issue is an artifact of my half-built sandbox. Also, we're not modifying the styles provided by Paragon here at all, so there's no custom trickery to fix.

So I'd like to proceed with merging here, and if this crops up again on the real sandbox, I'll look deeper.

@sdaitzman
Copy link

Hi @pomegranited, that sounds good to me. Thank you!

@pomegranited
Copy link
Contributor

@jmakowski1123 @lizc577 @sdaitzman @marcotuts This is ready for AC testing on the sandbox, including the latest changes.

See lib:OpenCraft:sortLib which contains these components:

  • "Problem (published)" -- created 1st, but modified 2nd.
  • "Text (published)" -- created 2nd but modified 1st. Published along with "Problem (published)".
  • "Text (unpublished)" -- created 3rd and modified 3rd
  • "Blank Problem" -- created 4th, unmodified

@lizc577
Copy link

lizc577 commented Sep 9, 2024

This looks good to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

No branches or pull requests

6 participants