-
Notifications
You must be signed in to change notification settings - Fork 205
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
Implement test search route and results view #3237
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
I imagined the presentation of the results more like on GitHub. So I'm wondering whether the Data table is a good choice here. And DataTable features like the almost immediate refreshing when entering something to the search input box and filtering might not work well for this kind of extensive search. The ticket also gives the idea that this search might return a lot of different types of results. I'm also wondering how well this would fit into a column-based structure. |
You're absolutely right. I started off with DataTables thinking it would be a flexible baseline and since we use it a lot and could re-use existing code. Unfortunately it's way more restrictive than I realized. So I changed it to render a bootstrap-styled list, with a text input at the top. And adding optional fields for different types of results is easy as well. |
3d34be2
to
aa79b2a
Compare
This pull request is now in conflicts. Could you fix it? 🙏 |
feb1081
to
3192029
Compare
Codecov Report
@@ Coverage Diff @@
## master #3237 +/- ##
==========================================
+ Coverage 91.49% 91.52% +0.03%
==========================================
Files 215 216 +1
Lines 13080 13108 +28
==========================================
+ Hits 11967 11997 +30
+ Misses 1113 1111 -2
Continue to review full report at Codecov.
|
Have you seen the OBS tests failing related to your changes?
because there is no git in OBS. I think that's something that we should be able to handle in a feasible way. How about just skipping the search in case of no git found and instead reporting the problem that a test repo without git is not supported or something. It might be easier to revert to manually traversing the filesystem. Pick whatever is easier for you to implement. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like I missed your last update and only react late, sorry about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just tested it locally with the search term "test". 37825 matches found. But at first it looked like nothing was found because there was no loading indication. Then the page was completely unresponsive for several seconds. There should be a loading indication and not everything should be rendered at once (e.g. let the JavaScript engine process events from time to time by splitting the loop or implement some endless scrolling).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems the most important points I've mentioned have been fixed. Especially the loading indication and avoiding the page hangs while rendering is still missing. But these improvements can be done in a follow-up PR.
c1a66f2
to
b5d3c53
Compare
Related: poo#34486