Skip to content

LG-5372 Follow on minor update to action button whitespace#5639

Merged
amathews-fs merged 2 commits intomainfrom
LG-5372_action_button_whitespace_fix
Nov 24, 2021
Merged

LG-5372 Follow on minor update to action button whitespace#5639
amathews-fs merged 2 commits intomainfrom
LG-5372_action_button_whitespace_fix

Conversation

@amathews-fs
Copy link
Contributor

LG-5372 Follow on minor update to action button top and bottom white space.

@amathews-fs amathews-fs requested a review from aduth November 24, 2021 19:01
url: url_for,
method: 'put',
html: { autocomplete: 'off', class: 'margin-top-2 js-consent-continue-form' } do |f| %>
<br/>
Copy link
Contributor

@aduth aduth Nov 24, 2021

Choose a reason for hiding this comment

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

🙏 to removal of <br>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't even see it when I was messing with all of that. I'm not even certain why it was all in there.

method: 'put',
html: { autocomplete: 'off', class: 'margin-top-2 js-consent-continue-form' } do |f| %>
<br/>
html: { autocomplete: 'off', class: 'margin-top-4 padding-bottom-1 js-consent-continue-form' } do |f| %>
Copy link
Contributor

Choose a reason for hiding this comment

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

If the idea is there should be 5 units of margin around the button, maybe we can do...

Suggested change
html: { autocomplete: 'off', class: 'margin-top-4 padding-bottom-1 js-consent-continue-form' } do |f| %>
html: { autocomplete: 'off', class: 'margin-y-5 js-consent-continue-form' } do |f| %>

This seems to collapse as expected.

The only problem is that there's extra padding on the last list item on the welcome step, but it seems like we can / should remove that.

url: url_for,
method: 'put',
html: { autocomplete: 'off', class: 'margin-top-2 js-consent-continue-form' } do |f| %>
html: { autocomplete: 'off', class: 'margin-top-2 padding-bottom-1 js-consent-continue-form' } do |f| %>
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar, we could do margin-bottom-5 and rely on collapsing margins.

Suggested change
html: { autocomplete: 'off', class: 'margin-top-2 padding-bottom-1 js-consent-continue-form' } do |f| %>
html: { autocomplete: 'off', class: 'margin-top-2 margin-bottom-5 js-consent-continue-form' } do |f| %>

Doesn't make much a difference, though I think could be easier to think about the form having 2.5rem bottom margin, vs. a combination of its 0.5rem bottom-padding and the next element's 2rem top-margin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mmmm, yeah, I get that. I should have been thinking about it as the large margin would set the distance. The margin collapsing thing is not something I think about normally.

@amathews-fs amathews-fs merged commit 8fbcfa0 into main Nov 24, 2021
@amathews-fs amathews-fs deleted the LG-5372_action_button_whitespace_fix branch November 24, 2021 21:50
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