search

helfi92 / material-ui-treeview

A React tree view for material-ui.
MIT License
47 stars 21 forks source link

Taskcluster 1754: [UI] Make search case insensitive #48

Closed Karska-dev closed 4 years ago

Karska-dev commented 4 years ago

Add parameter caseInsensitiveSearch

Change search functionality to use regular expression instead of string

Add descrition of new parameter caseInsensitiveSearch to ReadMe file

Github Bug/Issue: [UI] Make search case insensitive 1754

Checklist

Karska-dev commented 4 years ago

@helfi92 Please, review. I have a question. I didn't get what exactly is going on here https://github.com/helfi92/material-ui-treeview/blob/8e0e52e99aa91e21a287b2c3520f25ed2325ae01/src/components/MuiTreeView/index.jsx#L142-L148 performance optimization? caching? The question is: should a new parameter be added there too?

helfi92 commented 4 years ago

I didn't get what exactly is going on here

This basically just makes sure that logic doesn't always get calculated on each render. It just caches it and only recomputes it when any of the keys provided in the list of parameters change.

The question is: should a new parameter be added there too?

No need since the caching function calls filter which then looks at the case sensitive prop.

helfi92 commented 4 years ago

Released in material-ui-treeview@4.1.0 🎊

Karska-dev commented 4 years ago

Released in material-ui-treeview@4.1.0 🎊

Great! @helfi92 So, what's the plan now? Will be a new ticket for case sensitiveness in Taskcluster?

helfi92 commented 4 years ago

Every monday, our renovate bot in Taskcluster opens PRs to update packages. It will be updated then. If you want it to be done earlier, feel free to create a PR in the Taskcluster repo updating the package.

Karska-dev commented 4 years ago

Every monday, our renovate bot in Taskcluster opens PRs to update packages. It will be updated then. If you want it to be done earlier, feel free to create a PR in the Taskcluster repo updating the package.

@helfi92 Monday is ok for me. So, no issue need to be created for this, right? I'm just make a new PR for adding case insensitiveness for Hooks page and mention already closed issue #1754.

helfi92 commented 4 years ago

So, no issue need to be created for this, right?

Correct. Also, caseSensitiveSearch will default to false so there won't be any code changes in Taskcluster.

Karska-dev commented 4 years ago

So, no issue need to be created for this, right?

Correct. Also, caseSensitiveSearch will default to false so there won't be any code changes in Taskcluster.

Oh, looks like I finally got it. So, I just need to test it after renovate bot's PR will be merged and that's it?

helfi92 commented 4 years ago

Oh, looks like I finally got it. So, I just need to test it after renovate bot's PR will be merged and that's it?

Yep. I can try to flag you whenever that PR comes up.

Karska-dev commented 4 years ago

Oh, looks like I finally got it. So, I just need to test it after renovate bot's PR will be merged and that's it?

Yep. I can try to flag you whenever that PR comes up.

Thank you, but don't worry - I'll check it Monday night. Put it in my TODO.

helfi92 commented 4 years ago

Awesome :)

Karska-dev commented 4 years ago

@helfi92 Hello! I see commit from renovate bot, but I'm not sure that it was a right one. I do not see changes works here - https://community-tc.services.mozilla.com/hooks Does that environment has changes for Hooks incensetiveSearch already?

I cannot run my local copy again, has an error:


    throw err;
    ^

Error: Cannot find module '@hapi/hawk'```
helfi92 commented 4 years ago

I'm not sure why renovate didn't open a pull request for this. I've opened https://github.com/taskcluster/taskcluster/pull/2157. Do you want to pull that branch down and see if it works?

I cannot run my local copy again, has an error:

You'll want to run yarn in the root directory and inside the ui directory to install the new dependencies that have merged since the last time you interacted with that repo.

Karska-dev commented 4 years ago

I'm not sure why renovate didn't open a pull request for this. I've opened taskcluster/taskcluster#2157. Do you want to pull that branch down and see if it works?

Thank you for CC me, I'll check after merge will be done. I just trying to understand how thing are working in the project and check that code as well. Are you going to use escape regex for treeview as well?

You'll want to run yarn in the root directory and inside the ui directory to install the new dependencies that have merged since the last time you interacted with that repo.

My node version was too old. Fixed