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

More tiles on kiwix-serve welcome page (optimised) #786

Merged
merged 2 commits into from
Jul 20, 2022
Merged

Conversation

juuz0
Copy link
Collaborator

@juuz0 juuz0 commented Jun 23, 2022

Fixes kiwix/kiwix-tools#538

Before:
image
image

Now:
image
image

@juuz0
Copy link
Collaborator Author

juuz0 commented Jun 23, 2022

Though this can be tested from a user-perspective, I have to check something in code hence the WIP status 👍

Done.

@codecov
Copy link

codecov bot commented Jun 24, 2022

Codecov Report

Merging #786 (5aa74c6) into master (dfc6cad) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #786   +/-   ##
=======================================
  Coverage   70.65%   70.65%           
=======================================
  Files          53       53           
  Lines        3708     3708           
  Branches     2061     2061           
=======================================
  Hits         2620     2620           
  Misses       1086     1086           
  Partials        2        2           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dfc6cad...5aa74c6. Read the comment docs.

@juuz0 juuz0 marked this pull request as ready for review June 24, 2022 07:33
@juuz0 juuz0 requested a review from kelson42 June 24, 2022 07:33
@juuz0 juuz0 changed the title [WIP] Use available space for book element Use available space for book element Jun 24, 2022
@kelson42
Copy link
Collaborator

kelson42 commented Jun 25, 2022

@juuz0 Thank you for your PR, but I can’t believe this is the right approach. Far too complex and no padding change should be necessary. Either the padding is too big or the margin left and right are too big or the current conputation is wrong…. But it should be simpler to put n columns of tiles on a screen to fill it.

@mgautierfr
Copy link
Member

mgautierfr commented Jun 27, 2022

In https://svelte.dev/repl/2b6fd14cbd454b339dc8c55ea32d67e0?version=3.48.0 I've simply use a flexbox. The flexbox "computes" the best number of columns and adjust the padding/margin to have a proper layout.
There is no complex computation on our side.
You can try it by changing the width of the result preview.

@juuz0 juuz0 changed the title Use available space for book element optimize welcome page Jun 29, 2022
@juuz0
Copy link
Collaborator Author

juuz0 commented Jun 29, 2022

@kelson42 ping for re-review

@kelson42 kelson42 changed the title optimize welcome page More tiles on kiwix-serve welcome page (optimised) Jul 3, 2022
Copy link
Collaborator

@kelson42 kelson42 left a comment

Choose a reason for hiding this comment

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

@juuz0 Thank you for this new PR which seems really more reasonable. I few remarks:

  • My personal main problem was the problem visiblem on my iPad. It seems to be fixed now.
  • I have added a commit to better align the filters on the left. Hope this is OK for you.
  • I want the "number of books" title align left to the tile at the very left
  • On my computer I think the margin left and right are still too big (see screenshot). Why there is not one additional tile in the row?
    image

@juuz0
Copy link
Collaborator Author

juuz0 commented Jul 4, 2022

I have removed the centering layout, this is back to normal with a reduced left and right space.
Centring the tiles caused the problem with label, if there were 1 tile, 2 tiles, ....X tiles, it had to be changed all the time. Not easy to calculate correctly.

have added a commit to better align the filters on the left. Hope this is OK for you.

Yes, looks better.

image
image

@kelson42
Copy link
Collaborator

kelson42 commented Jul 5, 2022

@juuz0 This is what I get:
image

  • Margin right bigger than margin left
  • There is still place for one tile

I'm not against center the tiles, but secure the "X books" title is aligned to the most left tile

@juuz0
Copy link
Collaborator Author

juuz0 commented Jul 7, 2022

@kelson42 Review ping

@kelson42
Copy link
Collaborator

@juuz0 title is ok, but second problem is not fixed,
image

There is still plently of place for an additional tile in a row. Why you don't reduce the margins left and right?!

@juuz0
Copy link
Collaborator Author

juuz0 commented Jul 13, 2022

@kelson42 One more review ping :'D

juuz0 and others added 2 commits July 20, 2022 19:18
This change centers tiles on welcome page to give a more consistent whitespace look on both sides.
For this, the layout in Isotope JS is changed to masonry.
Copy link
Collaborator

@kelson42 kelson42 left a comment

Choose a reason for hiding this comment

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

From a user perspective, it finally works fine!

@kelson42 kelson42 merged commit 88c25b3 into master Jul 20, 2022
@kelson42 kelson42 deleted the optimizedWelcome branch July 20, 2022 17:41
mgautierfr added a commit that referenced this pull request Nov 30, 2022
* [API Break] Remove wrapper around libzim (@mgautierfr #789)
* Allow kiwix-serve to use custom resource files (@veloman-yunkan #779)
* Properly handle searchProtocolPrefix when rendering search result (@veloman-yunkan #823)
* Prevent search on multi language content (@veloman-yunkan #838)
* Use new `zim::Archive::getMediaCount` from libzim (@mgautierfr #836)
* Catalog:
 - Include tags in free text catalog search (@veloman-yunkan #802)
 - Illustration's url is based on book's uuid (@veloman-yunkan #804)
 - Cleanup of the opds-dumper (@veloman-yunkan #829)
 - Allow filtering of catalog content using multiple languages (@veloman-yunkan #841)
 - Make opds-dumper respect the namemapper (@mgautierfr #837)
* Server:
 - Correctly handle `\` in suggestion json generation (@veloman-yunkan #843)
 - Better http caching (@veloman-yunkan #833)
 - Make `/suggest` endpoint thread-safe (@veloman-yunkan #834)
 - Better redirection of main page (@veloman-yunkan #827)
 - Remove jquery (@mgautierfr @juuz0 #796)
 - Better Viewer of zim content :
   . Introduce `/content` endpoints (@veloman-yunkan #806)
   . Switch to iframe based content viewer (@veloman-yunkan #716)
 - Optimised design of the welcome page:
   . Alignement (@juuz0 @kelson42 #786)
   . Exit download modal on pressing escape key (@Juzz0 #800)
   . Add favicon for different devices (@Juzz0 #805)
   . Fix auto hidding of the toolbar (@veloman-yunkan #821)
   . Allow user to filter books by tags in the front page (@juuz0 #711)
* CI :
  - Trigger CI on pull_request (@kelson42 #791)
  - Drop Ubuntu Impish packaging (@legoktm #825)
  - Add Ubuntu Kinetic packaging (@legoktm #801)
* Testing:
  - Test ICULanguageInfo (@veloman-yunkan #795)
  - Introduce fake `test` language to test i18n (@veloman-yunkan #848)
* Fix documentation (@kelson42 #816)
* Udpate translation (#787 #839 #847)
@mgautierfr mgautierfr mentioned this pull request Nov 30, 2022
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.

Kiwix-serve welcome page not optimised
3 participants