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

Add "starts with" search filter #617

Open
whyboris opened this issue Jan 1, 2021 · 23 comments · May be fixed by #832
Open

Add "starts with" search filter #617

whyboris opened this issue Jan 1, 2021 · 23 comments · May be fixed by #832
Labels
good first issue Good for newcomers

Comments

@whyboris
Copy link
Owner

whyboris commented Jan 1, 2021

Simple and easy. May be useful when you remember the beginning of the word, but don't want the app to find the substring match in the middle of other words in the file name.

Example:
abc def.mp4 will be matched with a search for de but not ef

@whyboris whyboris added the good first issue Good for newcomers label Jan 1, 2021
@kanwal019
Copy link

Can I give it a try ?

@whyboris
Copy link
Owner Author

Awesome! Let me know if you have any questions or would like any help (whether vague or detailed) 🤝

@kanwal019
Copy link

@whyboris Should we expect abc_def.mp4 and abc-def.mp4 to match with a search for de but not ef, as well ?

@whyboris
Copy link
Owner Author

whyboris commented Jan 15, 2021

Thank you for the question!

Feels like _ for sure is meant as a space, so abc_def should be equivalent to abc def (as if 2 words).

I'm not sure about abc-def but it feels like two words to me as well (in enough cases that we may as well pretend it's 2 words).

I don't have a strong opinion on the -. Please implement as you recommend 🤝

One option is to simply use the cleanName rather than file name 🤔
https://github.com/whyboris/Video-Hub-App/blob/main/interfaces/final-object.interface.ts#L28
which already replaces . and _ with spaces:

export function cleanUpFileName(original: string): string {

(but it does not touch the -)

@kanwal019
Copy link

kanwal019 commented Jan 18, 2021

Thanks @whyboris.

Currently the filter works for both file name and partial path. Would it be ok , just to make it work for the cleanName ?
If so, can I update that to handle - as well ?

kanwal019 added a commit to kanwal019/Video-Hub-App that referenced this issue Jan 18, 2021
@whyboris
Copy link
Owner Author

whyboris commented Jan 19, 2021

I think the easiest (from code perspective and conceptual simplicity for the user) this feature (in this Issue) can just use cleanName for the search. You can avoid adding any extra logic around - (unless you want to add it -- up to you).

I was guessing that a simple way to do it would be to create a new pipe that will break up the cleanName string into an array of strings (.split(' ')) and then loop through each array with a .startsWith 🤷

The biggest challenge I see with this feature is going around all the files and correctly adding the relevant pieces for the UI to show the field, for appropriate text to appear next to the search/filter field, etc.

Let me know if you'd like any help 🤝

@whyboris
Copy link
Owner Author

ps - quick note about https://github.com/kanwal019/Video-Hub-App/pull/1/files#diff-84a1283509603799dadbc0d1e879272e0cd9196aa1bd092948cbdf07ba60eca7R279

Feel free to make any changes you'd like for your own fork and/or copy of the software, but I would not want to merge a change (at least at this time) that replaces the - in file names with spaces for the cleanName field (as you have in your PR linked above) 😓

@kanwal019
Copy link

Sure @whyboris I will have a separate branch for the fix. And I thought the PR was linked just to my copy of the code.

kanwal019 added a commit to kanwal019/Video-Hub-App that referenced this issue Jan 22, 2021
kanwal019 added a commit to kanwal019/Video-Hub-App that referenced this issue Jan 22, 2021
@kanwal019
Copy link

I think the easiest (from code perspective and conceptual simplicity for the user) this feature (in this Issue) can just use cleanName for the search. You can avoid adding any extra logic around - (unless you want to add it -- up to you).

I was guessing that a simple way to do it would be to create a new pipe that will break up the cleanName string into an array of strings (.split(' ')) and then loop through each array with a .startsWith 🤷

The biggest challenge I see with this feature is going around all the files and correctly adding the relevant pieces for the UI to show the field, for appropriate text to appear next to the search/filter field, etc.

Let me know if you'd like any help 🤝

This will work as expected in the case of magic-search, but how do we need to manage that for the file-search, as we can't just use cleanName to filter out the results.

@whyboris
Copy link
Owner Author

whyboris commented Jan 26, 2021

Sorry I'm unsure if I fully understand. My thought was an addition of a new filter, not editing of any other filter. This would mean all old filters would continue to behave as before with no change.

The new filter, if enabled, would have its own <input> field which, when users would type in and press the Enter key would perform the search (just like the current file search does). With the code I already have in the app, you might not need to edit any HTML either - it might be enough to just add another entry into the filter array (and update some other appropriate places).

To make this work we'd need also to create a new .pipe file with its own logic.

Please let me know if you're unsure about what I'm requesting with this feature, or about the implementation that I'm describing, or anything else 🤝

Thank you for your continued interest 🙇 though of course no worries if you ever want out 👌

@kanwal019
Copy link

I guess I misunderstood the requirement before. I would try to give a shot to the above approach and revert. Thank You @whyboris .

@kanwal019
Copy link

@whyboris I think, I need to spend some further time with the code to understand it. If someone else can take up on the issue, it will be better :(

@whyboris
Copy link
Owner Author

whyboris commented Feb 1, 2021

No worries at all! 👌
If you'd like me to guide you through the changes needed - I'm happy to do it too. If this is the first time you're using Angular, it can be overwhelming. And my code base might not be a paragon of organization. So far I gave you a vague outline of what's to be done, but I can give you a file-by-file description too if you're interested.
Just let me know 🤝 -- no pressure in either direction (whether you'd like someone else to do it, to have me help you in a more directed fashion, or something else) 😁

@kanwal019
Copy link

I would need to dedicate some time to get familiar with the code base. Probably, after that I would be in a condition to contribute towards the application.

@whyboris
Copy link
Owner Author

whyboris commented Feb 5, 2021

No worries at all 👌
I'm always happy to answer questions -- feel free to reach out here or over email 👍

@mschristo
Copy link

Hello!

may I give it a try too?

@whyboris
Copy link
Owner Author

@mschristo - thank you for the offer! Please give it a try & feel free to reach out with any questions 🤝

ps - if you ask for help, let me know if you'd prefer generic help (an outline of how things can be solved), or specific help (point to files and lines of code), or something in-between. I wouldn't want to ruin a good puzzle 😁

@Herty94
Copy link

Herty94 commented Oct 8, 2023

Is this still relevant ? I would like to contribute on this issue 😄

@whyboris
Copy link
Owner Author

Hey @Herty94 - I think it's still worth adding this small feature to the app 🤔
Do give it a try -- and I'm happy to help along with any step of the way 😊 👍

@Herty94 Herty94 linked a pull request Oct 24, 2023 that will close this issue
@Herty94
Copy link

Herty94 commented Oct 24, 2023

What's done:

  • So I've created a new search bar for start-with filter, added start-with pipe and also tried to include relevant icon.

Place for improvement:

  • I was uncertain about how to connect labels to translation object or how to append it.
  • The icon could be smaller and adjusted to complement others.
  • I didn't add a shortcut for start-with search because I was uncertain again about which key would be appropriate.

@whyboris
Copy link
Owner Author

whyboris commented Oct 24, 2023

Thank you 😄

Please only add to the en.json file (I will auto-translate to other languages 👌)

You could trace out how text labels connect by tracing it out. For example "video name has" in the sidebar

https://github.com/whyboris/Video-Hub-App/blob/main/i18n/en.json#L341

And the accompanying text in the settings menu:

https://github.com/whyboris/Video-Hub-App/blob/main/i18n/en.json#L48

Notice the pattern of [property name], [property name]Hint, and [property name]MoreInfo 👍 you can add more info and a "hint" (the i in settings to the side of the description) 😊

ps - I can mess with the icon to make it fit 👌
pps - no need for a keyboard shortcut for this search field 🤝

@Herty94
Copy link

Herty94 commented Nov 6, 2023

I've added translation and fixed formatting. I hope it's okay now 😊. You can check PR #832 .

@whyboris
Copy link
Owner Author

Thank you @Herty94 - I'll try to merge in the PR over the winter when I return to VHA development 🤞 ❤️ 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants