[go: up one dir, main page]

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

Fix wrong params url when removing the search text from search block #5615

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

Conversation

iFlameing
Copy link
Member

Fixes #5614 .

@sneridagh This is the same fix for the auto-populated issue. We just missed one case where we typed in the search box of searchblock and removed it.

Copy link
netlify bot commented Jan 11, 2024

Deploy Preview for plone-components canceled.

Name Link
🔨 Latest commit 6ccc4b7
🔍 Latest deploy log https://app.netlify.com/sites/plone-components/deploys/66f29070c293f00007af8b6d

Copy link
netlify bot commented Jan 11, 2024

Deploy Preview for volto canceled.

Name Link
🔨 Latest commit 25fedc1
🔍 Latest deploy log https://app.netlify.com/sites/volto/deploys/65a1400a518fb30009a036a9

@iFlameing iFlameing changed the title Fix Fix wrong params url when removing the search text from search block Fix wrong params url when removing the search text from search block Jan 11, 2024
@ichim-david
Copy link
Sponsor Member

I'll test this tomorrow morning, alongside #5262
It would be great to get both fixes merged soon if we are at search block fixes.

Copy link
Sponsor Member
@ichim-david ichim-david left a comment

Choose a reason for hiding this comment

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

@iFlameing @sneridagh
I tested locally this branch and I tried a demo on the plone demo.
Take into consideration that my experience with the search block is limited so I don't know all of the patches that went into it throughout time.

I tried to replicate the cypress test and I have the following
things to say:

https://demo.plone.org/search-block-test?query=%5B%7B%22i%22%3A%22portal_type%22%2C%22o%22%3A%22paqo.selection.any%22%2C%22v%22%3A%5B%22Document%22%2C%22Event%22%2C%22Folder%22%5D%7D%2C%7B%22i%22%3A%22portal_type%22%2C%22o%22%3A%22paqo.list.contains%22%2C%22v%22%3A%5B%22Document%22%2C%22Event%22%2C%22Folder%22%5D%7D%5D&sort_order=ascending

To me the URL parameters are modified just fine when clicking on the
clear input even before this code.
However even though the URL does not contain the searchText to Event
there is no loading of results afterwards.

loading

Locally this doesn't happen with your branch but neither does it happen with the main branch.

As such I'm still a bit confused on what this change fixes.
I am curious as to why you get the loading hickup on plone demo
vs locally with 18.

I will try a test also with Volto 17 locally and see if I can replicate
the demo issue sometime today but my initial review is:
doesn't seem to harm but doesn't seem to fix either :)

EDIT:
here is the location URL for the demo URL in default, then with Event searched, and Clear input
location-search-default-event-clear

To me the URLs parameters are fine unless I need to construct the search block differently in which
case give a more specific test case to test.

@fredvd
Copy link
Sponsor Member
fredvd commented May 15, 2024

@iFlameing Status? (working on a search block implementation and checking for bugs, found this one)

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

Successfully merging this pull request may close these issues.

Wrong params url when removing the search text from search block.
4 participants