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

Fix sidebar search, post bootstrap #1314

Merged
merged 3 commits into from
Apr 29, 2022
Merged

Conversation

kinow
Copy link
Collaborator

@kinow kinow commented Apr 28, 2022

Reasons for creating this PR

Bootstrap upgrade PR broke the sidebar search (completely forgot to test this part during the Bootstrap upgrade! Sorry 😨 )

Link to relevant issue(s), if any

Description of the changes in this PR

Fix sidebar search, by matching previous CSS styles. Had to modify the structure of HTML, and copy the btn-default style from Bootstrap 3.

Known problems or uncertainties in this PR

Checklist

  • phpUnit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works (if not, explain why below)
  • The PR doesn't introduce unintended code changes (e.g. empty lines or useless reindentation)

@kinow
Copy link
Collaborator Author

kinow commented Apr 28, 2022

This will need some testing locally or on finto dev. Here's what it looks like for me, running with Docker.

image

After selecting some items.

image

After applying the filters (had to include YSO vocab, d'oh).

image

@codecov
Copy link

codecov bot commented Apr 28, 2022

Codecov Report

Merging #1314 (3ccc395) into master (58d75db) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #1314   +/-   ##
=========================================
  Coverage     70.36%   70.36%           
  Complexity     1649     1649           
=========================================
  Files            32       32           
  Lines          3790     3790           
=========================================
  Hits           2667     2667           
  Misses         1123     1123           

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 58d75db...3ccc395. Read the comment docs.

@osma
Copy link
Member

osma commented Apr 28, 2022

Thanks a lot @kinow ! Will test...

@osma osma self-assigned this Apr 28, 2022
@osma osma added the bug label Apr 28, 2022
@osma osma added this to the 2.15 milestone Apr 28, 2022
@osma
Copy link
Member

osma commented Apr 28, 2022

I tested this briefly. Here you can see the old Skosmos 2.14 (left) and new (right):

kuva

EDIT: Changed the picture, I first pasted in the wrong one by mistake.

It's working well now!

The font color is different for the options - it used to be blue (link color) but now it's black. I think the black color is better actually. These are not links so there is no reason to use blue color. Also the highlighting is different, but here too I think the new look is better.

For the group selection, I suggest changing the square checkboxes to round radio buttons, as they used to be. This is because only one selection is allowed. For the others, the square checkboxes are good.

In general there is a little bit more vertical spacing in the new layout, for example between the labels and the selection boxes (highlighted on picture). It's the same with the search results seen on the right - they take up more space than they used to. For example the headline "catacombs" is visible in the old layout on the left side but not in the new layout on the right side. I wonder if there is some general CSS setting (line-height maybe?) that is different in Bootstrap 5 than it used to be in Bootstrap 3? This isn't super critical, just something I wonder about because it seems to affect all page types, not just the search result page shown here.

@kinow
Copy link
Collaborator Author

kinow commented Apr 28, 2022

For the group selection, I suggest changing the square checkboxes to round radio buttons, as they used to be. This is because only one selection is allowed. For the others, the square checkboxes are good.

👍

In general there is a little bit more vertical spacing in the new layout, for example between the labels and the selection boxes (highlighted on picture). It's the same with the search results seen on the right - they take up more space than they used to. For example the headline "catacombs" is visible in the old layout on the left side but not in the new layout on the right side. I wonder if there is some general CSS setting (line-height maybe?) that is different in Bootstrap 5 than it used to be in Bootstrap 3? This isn't super critical, just something I wonder about because it seems to affect all page types, not just the search result page shown here.

I will spend some time touching up after fixing the checkbox/radiobutton, trying to make it look more similar to finto. Can probably finish it tomorrow morning (not working, and my soccer team is playing in Sao Paulo, so eyes on the screen, ears to the radio ⚽ )

Thanks for testing it so quickly @osma!

@kinow
Copy link
Collaborator Author

kinow commented Apr 28, 2022

For the group selection, I suggest changing the square checkboxes to round radio buttons, as they used to be. This is because only one selection is allowed. For the others, the square checkboxes are good.

Huh, interesting. Looking at the HTML DOM, the "By group" fields are actually using type="radio".

image

One CSS rule that I added to have sharp corners in the checkboxes ended up causing the radio inputs to have sharp corners too. Essentially becoming check boxes 😆 Fixed. Now looking at the remaining style issues.

@kinow kinow force-pushed the fix-search-bootstrap branch from 46a8b00 to c2cf6c9 Compare April 28, 2022 19:52
@sonarcloud
Copy link

sonarcloud bot commented Apr 28, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
2.4% 2.4% Duplication

@kinow
Copy link
Collaborator Author

kinow commented Apr 28, 2022

In general there is a little bit more vertical spacing in the new layout, for example between the labels and the selection boxes (highlighted on picture). It's the same with the search results seen on the right - they take up more space than they used to. For example the headline "catacombs" is visible in the old layout on the left side but not in the new layout on the right side. I wonder if there is some general CSS setting (line-height maybe?) that is different in Bootstrap 5 than it used to be in Bootstrap 3? This isn't super critical, just something I wonder about because it seems to affect all page types, not just the search result page shown here.

Bootstrap 5 had a class .form-label with margin-bottom: .5em. But Bootstrap 3 had a fixed margin of 5px. I've copied the Bootstrap 3 setting over. The downside is when zooming in the margin won't be adjusted, which goes against accessibility practices, like WCAG. But now the look and feel should be more similar.

Some screenshots.

image

image

Had a look at the Feedback form and at the search language switcher to see if these changes broke those <select> inputs, but everything looks the same to me.

Ready for review again 🎉
Thanks
Bruno

@osma
Copy link
Member

osma commented Apr 29, 2022

Excellent work @kinow! All good now, I'm merging this.

@osma osma merged commit a54f285 into NatLibFi:master Apr 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Search options/filters broken after Bootstrap 5 upgrade
2 participants