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 headers to search results #5358

Merged
merged 1 commit into from
Nov 13, 2023
Merged

Conversation

perlpunk
Copy link
Contributor

@perlpunk perlpunk commented Nov 7, 2023

Now one can easily jump to the results by type and see the individual number of results

Slightly related issue: https://progress.opensuse.org/issues/137243

Screenshot:
openqa-searchresults

@perlpunk
Copy link
Contributor Author

perlpunk commented Nov 7, 2023

I know that the headers don't look very nice yet, but at least the information is there, and the data structure returned makes it easier now to deal with different kinds of results, so I would leave it to other pull requests to make it look nicer. I just wanted this for a long time and now did it :)

Copy link

codecov bot commented Nov 7, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9e4ee2f) 98.32% compared to head (52a9b3a) 98.32%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #5358   +/-   ##
=======================================
  Coverage   98.32%   98.32%           
=======================================
  Files         389      389           
  Lines       37320    37326    +6     
=======================================
+ Hits        36694    36700    +6     
  Misses        626      626           
Files Coverage Δ
lib/OpenQA/WebAPI/Controller/API/V1/Search.pm 97.61% <100.00%> (+0.15%) ⬆️
t/api/15-search.t 100.00% <100.00%> (ø)
t/ui/15-search.t 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@kalikiana kalikiana left a comment

Choose a reason for hiding this comment

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

I know that the headers don't look very nice yet, but at least the information is there, and the data structure returned makes it easier now to deal with different kinds of results, so I would leave it to other pull requests to make it look nicer. I just wanted this for a long time and now did it :)

Awesome. Simple but effective.

I agree styling doesn't need to be added here, and I like that you're going for the smallest step here. Not to mention this will also make it easier to add more types when we don't need to solve the whole filtering UX thing in one go.

okurz
okurz previously requested changes Nov 7, 2023
lib/OpenQA/WebAPI/Controller/API/V1/Search.pm Outdated Show resolved Hide resolved
assets/javascripts/openqa.js Outdated Show resolved Hide resolved
item.appendChild(contents);
const types = ['code', 'modules', 'templates'];

for (var i = 0; i < types.length; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

And this var should be a let.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it into an iterator

Copy link
Member

@kraih kraih Nov 7, 2023

Choose a reason for hiding this comment

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

I find loops cleaner looking personally. Arrow functions are now also preferable to use whenever possible, instead of the keyword function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the variable into an object and didn't know how to do it in a for loop :)

@perlpunk
Copy link
Contributor Author

perlpunk commented Nov 7, 2023

The headers are now h3, easier to read now

Now one can easily jump to the results by type and see the individual number
of results

Slightly related issue: https://progress.opensuse.org/issues/137243
@perlpunk perlpunk requested a review from okurz November 8, 2023 11:13
item.appendChild(contents);
const types = {code: 'Test modules', modules: 'Job modules', templates: 'Job Templates'};

Object.keys(types).forEach(function (searchtype) {
Copy link
Member

@kraih kraih Nov 8, 2023

Choose a reason for hiding this comment

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

for (const searchtype of Object.keys(types)) {

or

for (const [searchtype, searchvalue] of Object.entries(types)) {

@perlpunk perlpunk dismissed okurz’s stale review November 13, 2023 18:14

Not responding

@perlpunk
Copy link
Contributor Author

Please create followup-PRs for any improvements. I don't have a ticket for that and just wanted it solved. The kind of for loop is just a matter of taste.

@perlpunk perlpunk removed the request for review from okurz November 13, 2023 18:16
@mergify mergify bot merged commit 6e1f097 into os-autoinst:master Nov 13, 2023
13 checks passed
@perlpunk perlpunk deleted the search-headers branch November 13, 2023 18:26
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.

6 participants