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 grid-based stack placement for combined search #3077

Merged
merged 13 commits into from
Sep 28, 2023

Conversation

maccabeelevine
Copy link
Member

@maccabeelevine maccabeelevine commented Sep 7, 2023

This is a CSS Grid based layout for combined search, as an alternative to the existing flex-based modes. This has two primary advantages which I cared about:

  • combined.ini says "The order of sections in this file will control the display order of search results on screen." This is actually true only on wide screens. On mobile the columns are collapsed so that everything in column 1 displays above everything in column 2, etc. Grid mode actually does display things in the intended order regardless of width.
  • All columns in a given row get the same height, without any JS or additional CSS. This is a visual preference I see in many single-search implementations, and something our institution likely wants.

Of course there are other flexibilities you can get from a grid layout as well, such as more complex layouts of the individual search modules. I would probably leave those to local customizations, unless someone has a compelling idea for how to generalize it. But I do think that the basics of grid layout is worth including as an option in the core, for the advantages above and so it's easier to do those local customizations.

Now that all browsers support grid layout, which was obviously not the case when 'combined search' was developed, one could argue for actually replacing the existing 'distributed' mode. But I think it's best to leave it as-is for backwards compatibility, and because some people prefer the uneven / adaptive heights of each module.

composer.json Outdated Show resolved Hide resolved
@maccabeelevine maccabeelevine marked this pull request as ready for review September 8, 2023 13:53
Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Looks like this has some conflicts that need to be resolved (nothing hard -- just merge and rebuild the CSS). Other than that, it looks good to me, but since I'm not nearly the HTML/CSS expert that @crhallberg is, I'd be interested in hearing whether he has any suggestions before merging this.

@maccabeelevine
Copy link
Member Author

Looks like this has some conflicts that need to be resolved (nothing hard -- just merge and rebuild the CSS).

@demiankatz Yup lots of PRs lately with CSS changes. Fixed.

Other than that, it looks good to me, but since I'm not nearly the HTML/CSS expert that @crhallberg is, I'd be interested in hearing whether he has any suggestions before merging this.

Yes please!

Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

@maccabeelevine, I took a look at this from a visual perspective, and I notice that distributed mode and grid mode look nearly identical, except that distributed mode has more right margin. Is this intentional? If it's easy to change, it might make sense to align these so that they look identical except for the ordering. Screen shots:

Distributed:
image

Grid:
image

@maccabeelevine
Copy link
Member Author

distributed mode and grid mode look nearly identical, except that distributed mode has more right margin

@demiankatz Not intentional so much as the result of minimal CSS and I figured it was benign. But I agree it's better to match the look of distributed by default, since I've specified what were meant to be the only differences. The two modes should now have identical widths at all breakpoints.

Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

@maccabeelevine, I took a closer look at your new template and compared it against existing ones, which led me to notice some potential optimizations and possible bugs.

The potential optimizations were in code that was copied from other existing templates, so I opened #3118 to optimize everything.

themes/bootstrap3/templates/combined/stack-grid.phtml Outdated Show resolved Hide resolved
themes/bootstrap3/templates/combined/stack-grid.phtml Outdated Show resolved Hide resolved
themes/bootstrap3/templates/combined/stack-grid.phtml Outdated Show resolved Hide resolved
@maccabeelevine
Copy link
Member Author

The potential optimizations were in code that was copied from other existing templates,

@demiankatz Yes, exactly. I realized I was increasing the duplication among these stack-x templates but figured any refactoring should be done in a separate PR, as you're doing now.

@demiankatz demiankatz added this to the 9.1 milestone Sep 22, 2023
Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Thanks, @maccabeelevine -- this should be ready to merge, but I'll leave it open in case @crhallberg has a chance to give it a look. If you don't hear from him by Thursday, feel free to merge.

@maccabeelevine maccabeelevine merged commit 9935459 into vufind-org:dev Sep 28, 2023
6 checks passed
@maccabeelevine maccabeelevine deleted the combined-stack-grid branch September 28, 2023 13:03
EreMaijala pushed a commit to EreMaijala/vufind that referenced this pull request Dec 16, 2024
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.

2 participants