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

Developed filter and search functionality in the Cases Section #939

Closed
wants to merge 49 commits into from

Conversation

Swarnendu0123
Copy link
Contributor

@Swarnendu0123 Swarnendu0123 commented Feb 25, 2024

Fixes: #37

Description:

Developed filter and search functionality in the Cases Section.

Screenshot(s):

Screenshot (97)
Screenshot (98)

Video Demonstration:

26.02.2024_04.47.57_REC.mp4

📚 Documentation preview 📚: https://bowtie-json-schema--939.org.readthedocs.build/en/939/

@Julian
Copy link
Member

Julian commented Feb 26, 2024

Will take a closer look in the next few days, video looks nice though!

Copy link
Member

@Julian Julian left a comment

Choose a reason for hiding this comment

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

I've left a few comments on the code itself, but here are some other general ones:

  • As you type, the page jumps around. In particualr, when nothing matches (e.g. if you type asdf) the page sort of snaps quite unsatisfyingly, so that the search bar is at the bottom. It'd be a lot better if the position of the search bar didn't change as you type.
  • I think it'd be quite nice to have highlighing in some form -- in other words, while you type, it would be nice to highlight the parts of descriptions that are matching your live search in the accordions below.
  • I'm not 100% sure it'd help as I haven't used it, I'd just saved it to look at, but perhaps https://github.com/algolia/autocomplete can help us with this functionality.
  • It would be nice to be able to search also within the test description (as opposed to the test case description). I'm not sure whether we need a separate box for each, maybe we just always search through both? Then again I think it would be nice to also search through the schema and instance -- and there it does seem like you should be able to turn that on or off. Maybe we should have togglable buttons for "Search in: [ ] schema [ ] instance [ ] test [ ] test case".
  • Taking that yet further, probably it would be nice to have a dropdown which lets you select which implementations you want to see results for. But feel free to leave that for a next PR.

Thanks for your work so far, let me know what you think.

frontend/src/components/Cases/CasesSection.tsx Outdated Show resolved Hide resolved
frontend/src/components/Cases/CasesSection.tsx Outdated Show resolved Hide resolved
@Julian Julian added the waiting for author An issue or PR which is waiting for futher steps from its author label Feb 28, 2024
@Swarnendu0123
Copy link
Contributor Author

Swarnendu0123 commented Feb 28, 2024

  • As you type, the page jumps around. In particualr, when nothing matches (e.g. if you type asdf) the page sort of snaps quite unsatisfyingly, so that the search bar is at the bottom. It'd be a lot better if the position of the search bar didn't change as you type.

okay, so we can have a fixed scrollable container, inside which we can display all the tests, and if the search string is not matching to any tests, we can show no match found or something like that

  • I think it'd be quite nice to have highlighing in some form -- in other words, while you type, it would be nice to highlight the parts of descriptions that are matching your live search in the accordions below.

Yeah, great feature, let me try it.

Okey, that very great idea, 100% agreed, but I also have not used it before. And also I am not sure that I could implement that or not.

  • It would be nice to be able to search also within the test description (as opposed to the test case description). I'm not sure whether we need a separate box for each, maybe we just always search through both? Then again I think it would be nice to also search through the schema and instance -- and there it does seem like you should be able to turn that on or off. Maybe we should have togglable buttons for "Search in: [ ] schema [ ] instance [ ] test [ ] test case".

that is also an good idea.

  • Taking that yet further, probably it would be nice to have a dropdown which lets you select which implementations you want to see results for. But feel free to leave that for a next PR.

yeah, that's also sounds good!

Thanks for your work so far, let me know what you think.

Well, all ideas you proposed, it great to implement, I commit over this branch and ask you for suggestions :)

@Swarnendu0123
Copy link
Contributor Author

Swarnendu0123 commented Feb 28, 2024

What about the UI now?

  • added highlighter
  • fixed the search box jumping issue
28.02.2024_20.20.21_REC.mp4

@Swarnendu0123
Copy link
Contributor Author

It would be nice to be able to search also within the test description (as opposed to the test case description). I'm not sure whether we need a separate box for each, maybe we just always search through both? Then again I think it would be nice to also search through the schema and instance -- and there it does seem like you should be able to turn that on or off. Maybe we should have togglable buttons for "Search in: [ ] schema [ ] instance [ ] test [ ] test case".

I have an idea for enhancing the search functionality. Within the same SearchBox, users could specify the scope of their search. For instance, if someone wants to search within the schema, they could type schema: properties. This way, they can narrow down their search to specific areas such as properties in the schema. We could extend this functionality to other areas as well.

@Julian what do you suggest?

@Julian
Copy link
Member

Julian commented Feb 28, 2024

You're talking about Lucene syntax. That would certainly be nice yeah. Though I don't want to tackle too big of a bite all at once. I'd like to see something fully mergeable in <200 lines of changes, and if need be to add more functionality in follow-ups. So I fully believe making lucene syntax work is doable, and probably there are libraries that will help us do it just like e.g. GitHub search works, but yeah try not to go wild all at once unless you can get it all working well, it's more important that you work reliably -- your first version here was too hasty and didn't work well, so err on the side of "working even if less functionality".

@Swarnendu0123
Copy link
Contributor Author

I have changed the description type to description: string | JSX.Element. that's why I guess it is breaking the system again and again, let me update the corresponding files.

@Julian
Copy link
Member

Julian commented Feb 28, 2024

That sounds like a very strange change. But also again, work slow and correct rather than haphazardly.

@Swarnendu0123
Copy link
Contributor Author

That sounds like a very strange change. But also again, work slow and correct rather than haphazardly.

Can you please build my final commit locally and give me some suggestions, so that I can get an overview how to proceed?

@Julian
Copy link
Member

Julian commented Feb 28, 2024

Politely no. It's been less than an hour since we had a design discussion, and four hours total since we first discussed. There's no way that's enough time spent on your part just making sure you have as much worked out as you can before asking specific questions. Being direct, it makes it seem even more like laziness -- I've tried to subtly give you this feedback so I'll try less subtle now :). When I say "work more carefully" this is what I mean. Don't throw something poorly thought out over to me for me to work out the problems with. It's ok to get stuck, it's not ok to spend minimal time and then kick it over.

@harrel56
Copy link
Collaborator

Just to summarize slack discussion:
The only blocking issue is #939 (comment), if it's sorted out and the 2 mentioned cases pass, I think we can finally merge it. Also the lib for highlighting (https://www.npmjs.com/package/react-highlight-words) looks fine :)

@Swarnendu0123
Copy link
Contributor Author

Just to summarize slack discussion: The only blocking issue is #939 (comment), if it's sorted out and the 2 mentioned cases pass, I think we can finally merge it. Also the lib for highlighting (https://www.npmjs.com/package/react-highlight-words) looks fine :)

Yes! it is ready for merging! @Julian (Just mentioning you for the final review and merge)

@Julian
Copy link
Member

Julian commented Apr 21, 2024

@Swarnendu0123 Is that blocking issue resolved then?

@Swarnendu0123
Copy link
Contributor Author

Swarnendu0123 commented Apr 21, 2024

@Swarnendu0123 Is that blocking issue resolved then?

No, this the issue needs https://www.npmjs.com/package/react-highlight-words, this PR is already have changes ~190 lines. It will be hard to review if I implement it in the same PR. I will implement the lib in subsequent PR(s). We can merge it for now.

@Julian
Copy link
Member

Julian commented Apr 21, 2024

The word "blocking" means "this isn't usable without it" :) so that doesn't sound right, but I'll have a look myself at what state this is in at this point.

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

Successfully merging this pull request may close these issues.

Add (client-side) table filters and sorts to the report
3 participants