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

Change default dataset search mode to be recursive #7539

Merged
merged 4 commits into from
Jan 15, 2024

Conversation

daniel-wer
Copy link
Member

@daniel-wer daniel-wer commented Jan 10, 2024

As discussed internally.
The recursive prop already was true by default (see code), but parsing the url parameters on component mount effectively changed that to be false.

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Steps to test:

  • Move some datasets into a folder
  • Search for them from the root folder. The default search mode should be recursive and they should be found if there is a match
  • Reload the page while any of the three search modes is active -> It should still be active after a reload.

(Please delete unneeded items, merge only when none are left open)

@daniel-wer daniel-wer self-assigned this Jan 10, 2024
Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

thanks for looking into and fixing this! I only left one suggestion.

@@ -350,9 +350,9 @@ function useManagedUrlParams(
params.set("folderId", activeFolderId);
// The recursive property is only relevant when a folderId is specified.
if (searchRecursively) {
params.set("recursive", "true");
Copy link
Member

Choose a reason for hiding this comment

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

what do you think about keeping this line? then, the url would always define the recursive behavior and we could change the default in the future without changing the semantics of urls. I don't think it's important, but it feels a bit cleaner to me. your call!

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I like that 👍

@bulldozer-boy bulldozer-boy bot merged commit 44e058f into master Jan 15, 2024
2 checks passed
@bulldozer-boy bulldozer-boy bot deleted the change-dataset-search-default branch January 15, 2024 15:26
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.

None yet

2 participants