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

Adding margin to searchform #3865

Merged

Conversation

ThoWagen
Copy link
Contributor

As discussed in #3226 in this PR some margin is added to the searchform.

@demiankatz
Copy link
Member

Thanks, @ThoWagen!

@sturkel89, can you compare search box margins in all the themes and see if this fully addresses your feedback, or if any themes look odd for any reason?

@demiankatz demiankatz added this to the 10.1 milestone Aug 21, 2024
Copy link
Contributor

@sturkel89 sturkel89 left a comment

Choose a reason for hiding this comment

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

I've checked the padding around the search box in all five themes in both desktop and mobile. I see that the spacing above the box in bootstrap5 is improved from before.

This is nitpicky, but I would still recommend that we add a couple of pixels above the search box in both bootstrap themes to balance the spacing a bit in desktop view.

Also, I'd like to see a few pixels of padding above and below each element in both bootstrap themes, and maybe one additional pixel in bootprint3, to give them a little breathing room in mobile view.

I like the spacing in both desktop and mobile in both Sandal themes, with a few pixels above and below the search box, the dropdown, and the Find button. The white elements look very nice offset against the contrasting background.

Desktop:

Bootstrap3
image

Bootstrap5
image

Bootprint3
image

Sandal
image

Sandal5
image

Mobile

Bootstrap3
image

Bootstrap5
image

Bootprint3
image

Sandal
image

Sandal5
image

@ThoWagen
Copy link
Contributor Author

@sturkel89 thank you for your feedback! Just a few comments to clearify some points:

"This is nitpicky, but I would still recommend that we add a couple of pixels above the search box in both bootstrap themes to balance the spacing a bit in desktop view."
The padding depends on multiple configurations. For example the breadcrumbs also have some padding. Right now that gap on your screenshots seems to not fit the padding above the searchbox. So adding some padding to the top could fix that in this case. However, if the breadcrumbs are not sticky that fix would be contra productive.

Even when the breadcrumbs are not sticky and there are no search tabs the padding is not evenly distributed around the searchbox because the navbar has some minimum height. We could maybe center the searchbox vertically to fix that. But that might have other unwanted side effects.

My point is that we probably can not adjust the padding properly for each situation without an general overhaul of the navbar design. And that would be a lot of work.

"Also, I'd like to see a few pixels of padding above and below each element in both bootstrap themes, and maybe one additional pixel in bootprint3, to give them a little breathing room in mobile view."
This is not a specific to the sticky element feature but a general design change, right? I'm not against it but just wanted to make sure there is no misunderstanding.

@demiankatz
Copy link
Member

@sturkel89 and @ThoWagen: I agree that it is difficult to get padding perfect for every scenario without potential side effects. I would propose that we defer further tweaking until release 11.0, because at that point we will have removed all the bootstrap3-based themes, and it will be significantly less complicated to make further revisions. I'd welcome a JIRA ticket to track the issue if that would help.

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.

I've just resolved conflicts here so it's ready to merge. I'm approving it but will wait to hear from @sturkel89 before hitting the merge button. Thanks again!

@sturkel89
Copy link
Contributor

I agree that we can move forward with this PR as it is. Thanks for your work on this feature, Thomas!

@demiankatz demiankatz merged commit 9ecb064 into vufind-org:dev Aug 23, 2024
7 checks passed
LuomaJuha pushed a commit to LuomaJuha/NDL-VuFind2 that referenced this pull request Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants