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

Fixed 'Relevance' option not being selectable by site viewers in the search block's 'Sort on' #4213

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

JeffersonBledsoe
Copy link
Member

Changes the 'No selection' option in the 'Sort on' for the search block to instead be a selectable 'Relevance' option. If no text has been searched for, this is displayed as 'Unsorted'. Also includes the following minor changes to make the 'Relevance' option make more sense:

  • The 'Sort direction' buttons will be hidden if we're using 'Relevance/ unsorted'
  • The sorter won't be displayed if no options are set in 'Sort on options'
Screen.Recording.2023-01-03.at.5.01.49.pm.mov

Closes #4000

@netlify
Copy link

netlify bot commented Jan 3, 2023

Deploy Preview for volto canceled.

Name Link
🔨 Latest commit 2a96989
🔍 Latest deploy log https://app.netlify.com/sites/volto/deploys/641d8f5ef89caa00080c463f

@cypress
Copy link

cypress bot commented Jan 3, 2023

Passing run #4681 ↗︎

0 489 20 0 Flakiness 0

Details:

Merge remote-tracking branch 'origin/master' into sort-by-relevance
Project: Volto Commit: 2a96989bad
Status: Passed Duration: 12:40 💡
Started: Mar 24, 2023 2:07 PM Ended: Mar 24, 2023 2:19 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@JeffersonBledsoe JeffersonBledsoe changed the title Search block: Allow site viewers to select 'Relevance' as a sort option Fixed 'Relevance' option not being selectable by site viewers in the search block's 'Sort on' Jan 3, 2023
Copy link
Member

@sneridagh sneridagh left a comment

Choose a reason for hiding this comment

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

@JeffersonBledsoe LGTM, but instead of showing/hide the sort buttons I'd disable them (the Button component allows it using disabled), to avoid the UI "jump".

What do you think?

/cc @tiberiuichim

@sneridagh sneridagh requested a review from a team January 4, 2023 12:23
@tiberiuichim
Copy link
Contributor

@sneridagh Your proposal sounds good to me.

@JeffersonBledsoe
Copy link
Member Author

@sneridagh Updated to have the buttons disabled rather than hidden :)

@avoinea avoinea self-requested a review January 27, 2023 17:57
Copy link
Member

@sneridagh sneridagh left a comment

Choose a reason for hiding this comment

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

Taking a closer look now, and since we are updating the Left/Right/Top components and passing down the new prop. Wouldn't it be breaking? As it requires this update to existing shadows, if the SortOn component is changed and expects it.

@JeffersonBledsoe what happens if the prop are not passed down?

@JeffersonBledsoe
Copy link
Member Author

@sneridagh It should still safely fall back to the existing behaviour. I will do some more testing to verify this behaviour though!
But yes, to get this new behaviour it would require an update to shadows unfortunately.

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

Successfully merging this pull request may close these issues.

Default 'Relevance' sort order missing from search block's Sort on options
4 participants