layer5io / layer5

Layer5, expect more from your infrastructure
https://layer5.io
Apache License 2.0
817 stars 1.08k forks source link

Add search feature to blog page #2684

Closed kayceeDev closed 2 years ago

kayceeDev commented 2 years ago

Signed-off-by: Ekene Nwobodo nwobodoe71@gmail.com

Description

This PR fixes #623

Notes for Reviewers

Signed commits

l5io commented 2 years ago

πŸš€ Preview for commit a1701aa9867c06bf04aae34cf635be1f8f34ffa7 at: https://623f53a990645f5d72d6fe55--layer5ng.netlify.app

l5io commented 2 years ago

πŸš€ Preview for commit 3bfb9e50547e99b33bdb645c4494bc72ef53307c at: https://623f5c8ce851e35cb273f7dc--layer5ng.netlify.app

kayceeDev commented 2 years ago

@kayceeDev it will be great if you center align search field in the tab view : )

image

Thanks so much @Abhijay007 . I will adjust the styles.

kayceeDev commented 2 years ago

Please don't print the list in the search component only. If you make the search component dynamic, we can use it on many pages like blog, resources, news etc. @kayceeDev

They're reasons this method won't work. You know the search component is also filtering the state and then rendering the filtered state. This means that it only filters the state it's given and end up Saving it in the components state. There is no way to pass the state of a child to the parent.

The other alternative might be to have a general state that the search component filters and this might require redux and some other complex logic. We can have a chat about this if you think there are easy ways to achieve this.

YashKamboj commented 2 years ago

They're reasons this method won't work. You know the search component is also filtering the state and then rendering the filtered state. This means that it only filters the state it's given and end up Saving it in the components state. There is no way to pass the state of a child to the parent.

The other alternative might be to have a general state that the search component filters and this might require redux and some other complex logic. We can have a chat about this if you think there are easy ways to achieve this.

Ohh!! Thats why i was also stuck with the issue πŸ˜…πŸ˜….

kayceeDev commented 2 years ago

They're reasons this method won't work. You know the search component is also filtering the state and then rendering the filtered state. This means that it only filters the state it's given and end up Saving it in the components state. There is no way to pass the state of a child to the parent. The other alternative might be to have a general state that the search component filters and this might require redux and some other complex logic. We can have a chat about this if you think there are easy ways to achieve this.

Ohh!! Thats why i was also stuck with the issue πŸ˜…πŸ˜….

Haha. Well one step at a time.

subhradwip01 commented 2 years ago

Please don't print the list in the search component only. If you make the search component dynamic, we can use it on many pages like blog, resources, news etc. @kayceeDev

They're reasons this method won't work. You know the search component is also filtering the state and then rendering the filtered state. This means that it only filters the state it's given and end up Saving it in the components state. There is no way to pass the state of a child to the parent.

The other alternative might be to have a general state that the search component filters and this might require redux and some other complex logic. We can have a chat about this if you think there are easy ways to achieve this.

Maybe we can create a general search component that will take some props like data and a function from the blog component that function will just change the data inside the blog component on the basis of the search(filtered data that will be stored in the state of blog component) . In that way, we can use this search component in other components also.May be it sense

subhradwip01 commented 2 years ago

Please don't print the list in the search component only. If you make the search component dynamic, we can use it on many pages like blog, resources, news etc. @kayceeDev

They're reasons this method won't work. You know the search component is also filtering the state and then rendering the filtered state. This means that it only filters the state it's given and end up Saving it in the components state. There is no way to pass the state of a child to the parent. The other alternative might be to have a general state that the search component filters and this might require redux and some other complex logic. We can have a chat about this if you think there are easy ways to achieve this.

Maybe we can create a general search component that will take some props like data and a function from the blog component that function will just change the data inside the blog component on the basis of the search(filtered data that will be stored in the state of blog component) . In that way, we can use this search component in other components also.May be it sense

or we can create our own seach component without using js-seach package

kayceeDev commented 2 years ago

Please don't print the list in the search component only. If you make the search component dynamic, we can use it on many pages like blog, resources, news etc. @kayceeDev

They're reasons this method won't work. You know the search component is also filtering the state and then rendering the filtered state. This means that it only filters the state it's given and end up Saving it in the components state. There is no way to pass the state of a child to the parent. The other alternative might be to have a general state that the search component filters and this might require redux and some other complex logic. We can have a chat about this if you think there are easy ways to achieve this.

Maybe we can create a general search component that will take some props like data and a function from the blog component that function will just change the data inside the blog component on the basis of the search(filtered data that will be stored in the state of blog component). In that way, we can use this search component in other components also.Maybe it sense

Thanks so much. Your comments really gave me a nice intuition. I moved up the functions that performed the search in the parent component and passed on props that keep track of the state change into the search component and it works. I will also see if I can create a custom hook for the data searching. So that the custom hooks can actually be used whenever. I will see if this is possible. Nice job @subhradwip01 Screenshot from 2022-03-28 14-28-16

subhradwip01 commented 2 years ago

Please don't print the list in the search component only. If you make the search component dynamic, we can use it on many pages like blog, resources, news etc. @kayceeDev

They're reasons this method won't work. You know the search component is also filtering the state and then rendering the filtered state. This means that it only filters the state it's given and end up Saving it in the components state. There is no way to pass the state of a child to the parent. The other alternative might be to have a general state that the search component filters and this might require redux and some other complex logic. We can have a chat about this if you think there are easy ways to achieve this.

Maybe we can create a general search component that will take some props like data and a function from the blog component that function will just change the data inside the blog component on the basis of the search(filtered data that will be stored in the state of blog component) . In that way, we can use this search component in other components also.May be it sense

Thanks so much. You comments really gave me a nice intuition. I moved up the functions that performed the search in the parent component and passed on props that keeps track of the state change into the search component and it works. I will also see if I can create a custom hooks for the data searching. So that the custom hooks can actually be used whenever. I will see if this is possible. Nice job @subhradwip01 I am really happy to help you ☺️. You know I was also thinking to create a custome hook using js-search and use it in search component.

subhradwip01 commented 2 years ago

Please don't print the list in the search component only. If you make the search component dynamic, we can use it on many pages like blog, resources, news etc. @kayceeDev

They're reasons this method won't work. You know the search component is also filtering the state and then rendering the filtered state. This means that it only filters the state it's given and end up Saving it in the components state. There is no way to pass the state of a child to the parent. The other alternative might be to have a general state that the search component filters and this might require redux and some other complex logic. We can have a chat about this if you think there are easy ways to achieve this.

Maybe we can create a general search component that will take some props like data and a function from the blog component that function will just change the data inside the blog component on the basis of the search(filtered data that will be stored in the state of blog component) . In that way, we can use this search component in other components also.May be it sense

Thanks so much. You comments really gave me a nice intuition. I moved up the functions that performed the search in the parent component and passed on props that keeps track of the state change into the search component and it works. I will also see if I can create a custom hooks for the data searching. So that the custom hooks can actually be used whenever. I will see if this is possible. Nice job @subhradwip01 I am really happy to help you ☺️. You know I was also thinking to create a custome hook using js-search and use it in search component.

l5io commented 2 years ago

πŸš€ Preview for commit 860c232dad1ea5a72a0a927013d05671396ea407 at: https://6241e9f959850d0f3c4224da--layer5ng.netlify.app

subhradwip01 commented 2 years ago

It is not working for the list view... also I guess rather than using it in the sidebar it should be in the grid and list component as we visit the mobile view it is gone to the bottom of the page...search bar should be at top of the page..

kayceeDev commented 2 years ago

It is not working for the list view... also I guess rather than using it in the sidebar it should be in the grid and list component as we visit the mobile view it is gone to the bottom of the page...search bar should be at top of the page..

@Nikhil-Ladha said it is the desired implementation. That's why it's there. But I understand your point and will fix it to stay at the top.

l5io commented 2 years ago

πŸš€ Preview for commit 7a0d8ebf410454b625985c7b1bb83bd1572365a3 at: https://6243583ac2a862007b7c2db9--layer5ng.netlify.app

Nikhil-Ladha commented 2 years ago

It is not working for the list view... also I guess rather than using it in the sidebar it should be in the grid and list component as we visit the mobile view it is gone to the bottom of the page...search bar should be at top of the page..

@Nikhil-Ladha said it is the desired implementation. That's why it's there. But I understand your point and will fix it to stay at the top.

Yes..but it seems like we need to use the design which you shared earlier on slack, i e, at the top right of blogs gird/list.

kayceeDev commented 2 years ago

πŸš€ Preview for commit 7a0d8eb at: https://6243583ac2a862007b7c2db9--layer5ng.netlify.app

@Nikhil-Ladha this preview is the recent change with the search box at the top of the grid.

Nikhil-Ladha commented 2 years ago

rocket Preview for commit 7a0d8eb at: https://6243583ac2a862007b7c2db9--layer5ng.netlify.app

@Nikhil-Ladha this preview is the recent change with the search box at the top of the grid.

Ok But, I can't seem to understand on what basis is the search happening?

kayceeDev commented 2 years ago

rocket Preview for commit 7a0d8eb at: https://6243583ac2a862007b7c2db9--layer5ng.netlify.app

@Nikhil-Ladha this preview is the recent change with the search box at the top of the grid.

Ok But, I can't seem to understand on what basis is the search happening?

It searches the rendered blogs card by title and returns a match. So if a blog has the search input in its title? It renders only the matched cards. If you need more context, you can elaborate please.

Nikhil-Ladha commented 2 years ago

rocket Preview for commit 7a0d8eb at: https://6243583ac2a862007b7c2db9--layer5ng.netlify.app

@Nikhil-Ladha this preview is the recent change with the search box at the top of the grid.

Ok But, I can't seem to understand on what basis is the search happening?

It searches the rendered blogs card by title and returns a match. So if a blog has the search input in its title? It renders only the matched cards. If you need more context, you can elaborate please.

Sure. Issues that I noticed:

  1. We get only 1 result (which is not true).
  2. If the search term entered doesn't match any data, it shows the complete list (instead it should show a text like "Sorry no matches" or something better)
  3. The search doesn't happen on the complete data, and only on the blogs which are on that page.

The above are some initial findings, more can be discussed on Monday's call (if possible do fix the above before that πŸ˜… )

kayceeDev commented 2 years ago

rocket Preview for commit 7a0d8eb at: https://6243583ac2a862007b7c2db9--layer5ng.netlify.app

@Nikhil-Ladha this preview is the recent change with the search box at the top of the grid.

Ok But, I can't seem to understand on what basis is the search happening?

It searches the rendered blogs card by title and returns a match. So if a blog has the search input in its title? It renders only the matched cards. If you need more context, you can elaborate please.

Sure. Issues that I noticed:

  1. We get only 1 result (which is not true).
  2. If the search term entered doesn't match any data, it shows the complete list (instead it should show a text like "Sorry no matches" or something better)
  3. The search doesn't happen on the complete data, and only on the blogs which are on that page.

The above are some initial findings, more can be discussed on Monday's call (if possible do fix the above before that πŸ˜… )

For the first observations, I'd look into it possibly before the next call. Thanks very much.

kayceeDev commented 2 years ago

rocket Preview for commit 7a0d8eb at: https://6243583ac2a862007b7c2db9--layer5ng.netlify.app

@Nikhil-Ladha this preview is the recent change with the search box at the top of the grid.

Ok But, I can't seem to understand on what basis is the search happening?

It searches the rendered blogs card by title and returns a match. So if a blog has the search input in its title? It renders only the matched cards. If you need more context, you can elaborate please.

Sure. Issues that I noticed:

  1. We get only 1 result (which is not true).
  2. If the search term entered doesn't match any data, it shows the complete list (instead it should show a text like "Sorry no matches" or something better)
  3. The search doesn't happen on the complete data, and only on the blogs which are on that page.

The above are some initial findings, more can be discussed on Monday's call (if possible do fix the above before that sweat_smile )

So on your observations, I have made changes now and the search no longer returns just one result but all the matches. I will create something that renders when a search is not found. for the last part, the data on the page is paginated and the search only works in the page context. What I mean here is that the graphQL query returns only the requested data when a user toggles to the next page and feeds it into the blog component and this is the data that the search box only sees and filters when a user searches for a blog.

It will be great if there is a way to query the data all at once and index it with the search plugin so that the search value goes through the whole data and not just the only data fed into the component on each page toggle. Pardon me if my explanations seems verbose

leecalcote commented 2 years ago

I am so very much looking forward to this functionality.

shivaylamba commented 2 years ago

My suggestion is to use a more powerful open source search engine like https://meilisearch.com. Would like to know your thoughts @leecalcote @Nikhil-Ladha

kayceeDev commented 2 years ago

My suggestion is to use a more powerful open source search engine like https://meilisearch.com. Would like to know your thoughts @leecalcote @Nikhil-Ladha

The search plugin is not really the problem. I will discuss it in the next website meeting

subhradwip01 commented 2 years ago

My suggestion is to use a more powerful open source search engine like https://meilisearch.com. Would like to know your thoughts @leecalcote @Nikhil-Ladha

The search plugin is not really the problem. I will discuss it in the next website meeting

Yes, lets discuss on next website call for more clarity

l5io commented 2 years ago

πŸš€ Preview for commit 803f9516c300a17594d05f942163402a959a667a at: https://6246f594c474552a7c7f155e--layer5ng.netlify.app

Nikhil-Ladha commented 2 years ago

Hey, @kayceeDev any updates from the Website's call? What's the next step?

kayceeDev commented 2 years ago

Hey, @kayceeDev any updates from the Website's call? What's the next step?

Hi @Nikhil-Ladha, since the graphql query is getting data in 10s, and passing it onto the page context, that's why the search doesn't index the whole data. So it was suggested that I should adjust the query to get the all the data, so it can all be indexed by the search plugin. Also, @shivaylamba pointed me to other search plugins that might be more efficient. I intend to incorporate all the feedback and see if it works as intended.

l5io commented 2 years ago

πŸš€ Preview for commit 9cc08b42f83734d955c7e05b409fc00d353b6df1 at: https://6254b02bc23e833bb0f37d57--layer5ng.netlify.app

l5io commented 2 years ago

πŸš€ Preview for commit 153b57d482673c249765a68f9c56110aaebecd57 at: https://6254bce863f1a1481fc44948--layer5ng.netlify.app

Nikhil-Ladha commented 2 years ago

Few issues:

  1. Paging is not working if the search results expand multi-page.
  2. Paging for any of the Tags, Categories is not working.
  3. Why have you increased the number of items per page to 12, 24 (in search results)? Please revert it to 10.
kayceeDev commented 2 years ago

This was also discussed in the last meeting. I will update as required and update PR. Also, if merge can be considered for this PR that will be fine so I will create different PRs for all the updates above so that the PR won't get long.

Nikhil-Ladha commented 2 years ago

This was also discussed in the last meeting. I will update as required and update PR. Also, if merge can be considered for this PR that will be fine so I will create different PRs for all the updates above so that the PR won't get long.

You can update this PR itself, no issues. I have gone through the code once everything seems fine. Not much trouble, will go again when the functionality is working as expected. Some basic functionalities are not working as expected, so I would prefer to fix that before a merge.

kayceeDev commented 2 years ago

Alright.

debo19 commented 2 years ago

@kayceeDev Can we discuss your updates on the call today?

l5io commented 2 years ago

πŸš€ Preview for commit 5daa6efbcb2e2c0f1203b5e7bf295d907bb5d9b7 at: https://6271420fbf80591215945306--layer5ng.netlify.app

l5io commented 2 years ago

πŸš€ Preview for commit 2d6d9f65b4a9f6cde85a62a85d139a74fcfd10b0 at: https://62717784dc5a790b82a0842e--layer5ng.netlify.app

l5io commented 2 years ago

πŸš€ Preview for commit 85e008a719d70e65bab84ca92f13a41dbd367569 at: https://62717ee5dc5a7912aca08ada--layer5ng.netlify.app

subhradwip01 commented 2 years ago

LGTMπŸ˜ƒ

leecalcote commented 2 years ago

Say it ain't so... is Search ready to go?!? :)

leecalcote commented 2 years ago

Search isn't supported on /resources, yet, huh?

We don't give a count of the number of matching articles, no?

We don't have a site-wide search yet, eh?

subhradwip01 commented 2 years ago

Search isn't supported on /resources, yet, huh?

We don't give a count of the number of matching articles, no?

We don't have a site-wide search yet, eh?

Hmm...good points....now we have a architecture for searching in blog page...we may use this for other pages...let discuss those things on next call so that we can create a more generalized form

kayceeDev commented 2 years ago

Search isn't supported on /resources, yet, huh?

We don't give a count of the number of matching articles, no?

We don't have a site-wide search yet, eh?

Search only works on the blog page for now. Adding the search to the resources page would be the same way it was added to the blog page but easier now since a lot of figuring out has been done.

Also, it won't be a site wide search (if I understand what you mean) since each page has different data coming in, but with the way I created the solution, the search feature can easily be added across any page that needs it.

kayceeDev commented 2 years ago

Search isn't supported on /resources, yet, huh?

We don't give a count of the number of matching articles, no?

We don't have a site-wide search yet, eh?

Also, we can actually give a count of the matching articles in the UI just like the behavior that happens when you are searching in the category or tag page.

Nikhil-Ladha commented 2 years ago

@leecalcote sometimes it may be hard to understand you ;), but in any way, I don't think you mind getting this merged and continuing improvements on top of it. Though, I will wait for your ack before merging this.

leecalcote commented 2 years ago

@leecalcote sometimes it may be hard to understand you ;), but in any way, I don't think you mind getting this merged and continuing improvements on top of it. Though, I will wait for your ack before merging this.

Thank goodness that you do so well deciphering my messages, @Nikhil-Ladha. Yes, that is a great plan. πŸ‘

@kayceeDev, confirmation: js-search was chosen over alternatives, because.... ? If this is written down somewhere, please just point me to the discussion.

leecalcote commented 2 years ago

How easy is it to propagate support for search site-wide using js-search vs an alternative library?

leecalcote commented 2 years ago

In some respects, irrespective of the answers to my questions above, it would be good to recognize (and merge) the work that has been done here, so that it can have a chance to be used, while be evaluated in context of those questions.

kayceeDev commented 2 years ago

Looks good. Good job @kayceeDev πŸ’―

Thanks @Nikhil-Ladha. You were really helpful.