-
Notifications
You must be signed in to change notification settings - Fork 17
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
Style organisation logos with inline-block instead of flexbox #3308
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change looks good to me overall, I like the approach of removing flex-basis
from mobile and tablet, and wonder if it would be best to remove flex-basis
from desktop as well. This would help keep the layout consistent across all screen sizes.
Non-blocking, but could also remove min-width and increase the margin-right to 30px to it still looks similar on desktop, and matches spacing between used on existing columns, something like that below for example:
.organisation-logos__logo {
padding-bottom: govuk-spacing(3);
margin-right: govuk-spacing(6);
}
Thanks @MartinJJones - was just thinking about removing Good idea with the |
I like this idea 👍 Looking at the existing code it looks like flex is used to try and create a 4 column layout, using flex-basis: 25% but this will only ever result is a max of 3 logos per row, because the margin-right: 15px increases it beyond the 25% width. This can be fixed, but will still likely result in more work to try and avoid the original issue. So going with an option to let the logos always take up the space needed and have consistent spacing between them sounds like a good option to me. |
3f9141d
to
2a3d7d8
Compare
2a3d7d8
to
de0adcc
Compare
This prevents an issue where organisation names were wrapping too early
de0adcc
to
8533392
Compare
Thanks @MartinJJones - i've made those changes. Here are the updated visual changes: BeforeAfterBeforeAfter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, looks like a nice improvement, especially the examples on mobile as well 👍
What / Why
flex-basis: 25%
, which restricts the width of the logoflex-basis
exists because some publications have more than one organisation logo on the page. For example: https://www.gov.uk/government/publications/10-downing-street-correspondence-communications-gifts-and-hospitality-privacy-notice/10-downing-street-correspondence-communications-gifts-and-hospitality-privacy-noticeflex-basis
is only needed on desktop, since the logos will stack on tablet / mobile. Therefore I have wrapped theflex-basis
in a desktop-only media query.flex-basis
to30%
, as thePrime Minister's Office
text was still wrapping while the screen size was within ourdesktop
width boundary.Visual Changes
Before (one logo example)
After (one logo example)
Before (multiple logo example)
After (multiple logo example)
Follow these steps if you are doing a Rails upgrade.