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

V8: Change "Include subfolders in search" to "Search only in this folder" #4720

Conversation

kjac
Copy link
Contributor

@kjac kjac commented Feb 24, 2019

Prerequisites

  • I have added steps to test this contribution in the description below

If there's an existing issue for this PR then this fixes: #4707

Description

#4707 outlines the requirements for this PR 😄 here's how it looks when applied:

search-only-this-folder

While I was at it I found some unused localization going on in the media picker controller, so I removed that as well.

@emmaburstow
Copy link
Contributor

Hiya Kenn,

Thanks for all the work on this. We look forward to testing it. Speak soon,

Em

@poornimanayar
Copy link
Contributor

Hey @kjac ,

Testing this on the starter kit and this what I noted. I searched for the word "Lee" from within the product folder in the media section , with the checkbox unchecked and I couldnt find the image. However if I am on the root (Media) I can find it. I can also find the image if I search for the word "Lee" in the people folder. Basically I cannot search for a media item in another folder if I am inside a folder. But isnt that the point of the checkbox, to limit it, rather than the search doing so? Am I getting the idea of the PR totally wrong here?

Poornima

@kjac
Copy link
Contributor Author

kjac commented Mar 20, 2019

Hi @poornimanayar. Sounds like a bug in the PR. Good job 👍

I'll investigate and get back.

@kjac
Copy link
Contributor Author

kjac commented Mar 20, 2019

@poornimanayar my bad, sorry. I misunderstood the intentions of #4707.

Here's how it's (hopefully) implemented now:

  1. When "Search only this folder" is selected, only the current folder is searched - any sub folders of the current folder are not included.
  2. When "Search only this folder" is not selected, the entire scope of the media picker is searched. This means:
    • If a start node is set for the media media picker, the entire tree under the start node is searched.
    • If a start node is not set for the media picker, the entire media library is searched (if a media start node is set for the current user, the search is limited to the tree under the start node).

@poornimanayar
Copy link
Contributor

Hey Kenn,

I set up 2 datatypes using Media Picker - one with a start folder and one without and here are my findings
Media Picker with no Start
Searching when on Media Folder

  • With "Search only this folder" checked it searches only the folder and not subfolders
  • else it searches everything

Searching when on Media/Design Folder(I choose a folder and start searching from there)

  • With "Search only this folder" checked it searches only the folder and not subfolders
  • With the checkbox unchecked it searches items & subfolders in "Media/Design" folder. But I cannot find something in Media/People folder. The test cases have made it so much more clear so I think it makes sense not to allow searching on a folder from within the context of another folder

Media Picker with Start

Searching when on Media/Design Folder(I choose a folder and start searching from there)

  • With "Search only this folder" checked it searches only the folder and not subfolders
  • With the checkbox unchecked it searches items & subfolders in "Design" folder.

Do you think I have covered it all?

Poornima

@kjac
Copy link
Contributor Author

kjac commented Mar 21, 2019

Hi again.

Could you re-verify your scenario Searching when on Media/Design Folder(I choose a folder and start searching from there) with the checkbox unchecked? When the checkbox is unchecked and there is no start node set for the media picker, it should search the entire media library regardless of your currently selected folder.

@poornimanayar
Copy link
Contributor

My turn to apologise @kjac ! Its all good. When I am within a folder, given the "search within this folder only" is unchecked, it searches the entire media library.

Great great work! I am just loving testing and reviewing your PRs, quite a PR hero you are pal!

Poornima

@kjac
Copy link
Contributor Author

kjac commented Mar 21, 2019

Whoo 😆

@nul800sebastiaan
Copy link
Member

Hey @kjac thanks for this so far! In #5356 @uniquelau makes a good suggestion, when checking on the checkbox, can we remember that for the current session so you don't have to keep enabling it for each search?

@kjac
Copy link
Contributor Author

kjac commented Jun 3, 2019

@nul800sebastiaan sure, shouldn't be a problem. I'll add it to this PR 👍

@kjac
Copy link
Contributor Author

kjac commented Jun 3, 2019

@nul800sebastiaan all done; works like this:

exclude-sub-folders-session-storage

@kjac
Copy link
Contributor Author

kjac commented Jul 13, 2019

Fixed merge conflicts

@kjac
Copy link
Contributor Author

kjac commented Jul 30, 2019

PR updated with latest v8/dev

@kjac
Copy link
Contributor Author

kjac commented Nov 15, 2019

PR updated with latest v8/dev

@emmaburstow emmaburstow changed the base branch from v8/dev to v8/contrib January 30, 2020 10:34
# Conflicts:
#	src/Umbraco.Web.UI.Client/src/views/common/infiniteeditors/mediapicker/mediapicker.controller.js
#	src/Umbraco.Web.UI.Client/src/views/common/infiniteeditors/mediapicker/mediapicker.html
#	src/Umbraco.Web.UI/Umbraco/config/lang/en_us.xml
@emmaburstow emmaburstow merged commit da20bf0 into umbraco:v8/contrib Jan 30, 2020
@kjac kjac deleted the v8-feature-reverse-media-picker-folder-search branch January 30, 2020 12:40
@ronaldbarendse
Copy link
Contributor

@kjac If I'm reading the comments correctly, it's not possible to search a part of the tree (anymore)?

So if you have the following structure:

  • People
    • Kenn Jacobsen
  • Products
    • Jackets
      • Umbracos Worldwide
    • Goodies
      • Sticker

After opening the Products folder and entering the search query jac, I would expect it to be scoped to the products and only return Jackets (and all descendants by default, only children if 'Search only this folder' is selected).

And should the plain checkbox be converted into a toggle?

@nielslyngsoe
Copy link
Member

nielslyngsoe commented May 28, 2020

Hi @kjac and @ronaldbarendse, Ronalds comment gave me some attention to this PR. I now understand that the wording "Search only this folder" could be misunderstood, which is the case here. My intention of the behavior was to mimic the behavior of Mac OS. When searching in Mac OS Finder you can choose to search the computer or the current folder. When searching the current folder you will find files in the given folder and files of sub-folders. I was dreaming about the same for this one.

I understand that it would have been nice if I had specified that in the issue, sorry for that. I also understand that some users might find the wording misleading, so inputs on wording are very welcome.

I do though think the current wording is okay — Users will try and learn how it works. Its all in the interpretation of what you think lies within a folder. I'm not feeling super strongly for it, but I'm currently feeling like 80% for that sub-folders is part of the given folder. But well, what does my opinion mean, so please give me your point of view on this, if you have some Content Managers around, could you then ask them? thanks! :-)

@kjac
Copy link
Contributor Author

kjac commented May 28, 2020

@nielslyngsoe right 👍 I'll dig into it and make sure that "Search only this folder" does indeed search the entire subtree below the current folder. The wording... well, it works - for now at least, until someone comes up with something better 😄

@kjac
Copy link
Contributor Author

kjac commented May 28, 2020

PR in #8197

@nielslyngsoe
Copy link
Member

@kjac Super great, thanks for looking into it this fast, then we will have it corrected for next release :-D

@nul800sebastiaan nul800sebastiaan added release/8.7.0 release/no-notes This is too small to add to the release notes or fixed after a beta/RC labels Sep 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release/no-notes This is too small to add to the release notes or fixed after a beta/RC release/8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

V8: Search functionality "include subfolders in search" is not very clear or understandable.
6 participants