-
Notifications
You must be signed in to change notification settings - Fork 20
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
Use flexbox for share links with square icons #4316
Conversation
564abd1
to
7a462bf
Compare
7a462bf
to
7947f62
Compare
7947f62
to
015d5ab
Compare
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 good! Got a couple of very minor comments. Also I tried a whole bunch of options trying to get this to work as per the designs (e.g. justify-content
, playing with auto margins, playing with a grid
layout) but I just couldn't get it to work nicely across all the different scenarios - I think this might be as close as we can get for the moment.
app/views/govuk_publishing_components/components/docs/share_links.yml
Outdated
Show resolved
Hide resolved
015d5ab
to
a2ab2be
Compare
a2ab2be
to
ac3462f
Compare
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.
Looks good to me 👍
What / Why
columns: true
for our square icons as it doesn't give us the visual balance we want where we're using it.min-width
for this variant instead.min-width
is needed because alignment/stacking visual inconsistencies occur when the elements have varying widths. Therefore, if we ensure there is a minimum width for the elements, the alignment and flow when resizing is more visually pleasing. If you think of a component like our image cards, they stack and align nicely as they are all the same width, so we need some of that guarantee here, hence themin-width
. It's also worth noting thatcolumns: true
was utilising amin-width
for its balancing of links. I don't think we should be putting any max width type limitations though likecolumns
does. (Mainly mentioning this as in the previous PR I originally usedmin-width
, which was what led to back and forth conversation and alternatives being suggested.)Visual Changes
Before
After