Skip to content

Unify whitespace on search pages with no results#10152

Merged
cdrini merged 7 commits intointernetarchive:masterfrom
purohitamann:10067/fix/search-result
Jan 8, 2025
Merged

Unify whitespace on search pages with no results#10152
cdrini merged 7 commits intointernetarchive:masterfrom
purohitamann:10067/fix/search-result

Conversation

@purohitamann
Copy link
Contributor

Closes #10067 Unify whitespace on search results pages (when no search results).

Fix - This PR ensures consistent whitespace when no search results are found on the search results pages.

Technical

Adjusted the CSS and layout for the "no search results" page state.
Verified styling consistency with other search result states.

Testing

To verify the fix:

Visit the search results page.
Enter a query that does not yield any results.
Observe the following:
Whitespace above and below the "no results" message is consistent.
The layout remains visually appealing and aligned.

Screenshot

Screenshot 2024-12-17 at 04 14 25 Screenshot 2024-12-17 at 04 14 16

Stakeholders

@RayBB

Copy link
Collaborator

@RayBB RayBB left a comment

Choose a reason for hiding this comment

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

@purohitamann this could be a potential solution but adds a lot of unneeded whitespace and doesn't deal with the root cause of there being extra whitespace on some pages like authors and subjects.
As described in the original issue.

We should unify them to all match the heights on the "all" and "lists" pages.

I'd recommend reverting the changes to v2.less and instead looking at the actual search results pages to understand where the extra space is coming from and remove it.

@purohitamann
Copy link
Contributor Author

@purohitamann this could be a potential solution but adds a lot of unneeded whitespace and doesn't deal with the root cause of there being extra whitespace on some pages like authors and subjects. As described in the original issue.

We should unify them to all match the heights on the "all" and "lists" pages.

I'd recommend reverting the changes to v2.less and instead looking at the actual search results pages to understand where the extra space is coming from and remove it.

noted. will make the necessary changes!

@purohitamann
Copy link
Contributor Author

@RayBB, I think this should do the job. I added a conditional check for empty author list results and subjects. let me know if anything needs to be changed. Thanks!

@RayBB
Copy link
Collaborator

RayBB commented Dec 17, 2024

Can you please provide a short video of how the pages look now?

@purohitamann
Copy link
Contributor Author

Here you go!
unify-whitespace

@RayBB
Copy link
Collaborator

RayBB commented Dec 17, 2024

@purohitamann the subjects/lists are still different height than the authors/search inside.
CleanShot 2024-12-17 at 16 27 22

@purohitamann
Copy link
Contributor Author

that's interesting! I will look into that and respond with a more effective solution.

@purohitamann
Copy link
Contributor Author

Clearfix div inside the pager macros is responsible for this issue. I removed the padding to address the issue
unify-whitespace

@RayBB
Copy link
Collaborator

RayBB commented Dec 17, 2024

If you think it is necessary to change the clearfix class, please check if it is used anywhere else and is changing it. Otherwise, you can try removing the clearfix. Please remove other changes if they are unnecessary.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 17.44%. Comparing base (347bff9) to head (58e6553).
Report is 159 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #10152      +/-   ##
==========================================
+ Coverage   17.12%   17.44%   +0.31%     
==========================================
  Files          89       89              
  Lines        4752     4792      +40     
  Branches      831      848      +17     
==========================================
+ Hits          814      836      +22     
- Misses       3428     3436       +8     
- Partials      510      520      +10     

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

@purohitamann
Copy link
Contributor Author

Removed the unnecessary clearfix div from the Pager component, as well as from the author and subject templates, to eliminate extra whitespace issues. reverted the padding configuration for the clearfix class to ensure other valid invocations across the codebase continue to work as expected without introducing layout inconsistencies.

whitespace

@purohitamann
Copy link
Contributor Author

@RayBB I’ve noticed that the Inside search page currently lacks a loading indicator. Should this be addressed to improve the user experience?

Copy link
Collaborator

@RayBB RayBB left a comment

Choose a reason for hiding this comment

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

Tested and the change is working perfectly.

Confirmed in both places we need the response.num_found because otherwise the ul will create whitespace.

Now we just need @cdrini to review/merge.

@RayBB RayBB added the Needs: Staff / Internal Reviewed a PR but don't have merge powers? Use this. label Dec 20, 2024
@RayBB
Copy link
Collaborator

RayBB commented Dec 20, 2024

@purohitamann good observation but a loading indicator isn't needed on that page because it will show the search inside results right away and doesn't need to load anything.

@RayBB RayBB changed the title 10067/fix/search result Unify whitespace on search results pages (when no search results). Dec 30, 2024
@RayBB RayBB changed the title Unify whitespace on search results pages (when no search results). Unify whitespace on search pages with results Dec 30, 2024
@RayBB RayBB changed the title Unify whitespace on search pages with results Unify whitespace on search pages with no results Dec 30, 2024
Copy link
Collaborator

@cdrini cdrini left a comment

Choose a reason for hiding this comment

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

Code lgtm and @RayBB tested. Awesome, thank you @purohitamann !

@cdrini cdrini merged commit f4ed723 into internetarchive:master Jan 8, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs: Staff / Internal Reviewed a PR but don't have merge powers? Use this.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Unify whitespace on search results pages (when no search results).

4 participants