-
Notifications
You must be signed in to change notification settings - Fork 43
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(ras-acc): apply terms and conditions styles #1910
base: epic/ras-acc
Are you sure you want to change the base?
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.
If it helps, I did get a thumbs up from Kevin for the styles used here -- the main difference is the font size and lack of background.
Personally I like how it looks now! The only thing that stands out to me with the label part is the distance between the text and the asterisks.
When the terms are open, the spacing is also a little funky -- the terms box is closer to the privacy policy text than the toggle:
I missed this myself, but when you add a heading the spacing at the top of the terms also seems like a lot! I think removing the top margin from the first element should take care of most cases of that.
136e231
to
ac5e438
Compare
Thanks for the review @laurelfulford! I've got a fix up for the terms box and abbreviation spacing in 850d5aa and ac5e438 Let me know if this works! |
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.
Thanks @chickenn00dle! This looks awesome overall! The only other thing I see is that the spacing next to the checkbox seems to be a bit smaller usual; it's only really noticeable when you have the checkboxes back to back like this:
I think it's just a matter of changing var(--newspack-ui-spacer-base,8px)
to var(--newspack-ui-spacer-2,12px)
to match the checkbox above; marking as approved since it's a small tweak and things otherwise are solid!
All Submissions:
Changes proposed in this Pull Request:
Closes https://app.asana.com/0/1207817176293825/1208579907739880
Looks like our terms and conditions styles weren't being applied correctly because we were targeting invalid classnames and nested elements. This PR adjusts our checkout styles to correctly target this section.
NOTE: I'm making some assumptions about what this should look like based on the existing styles since I didn't see this checkbox in the Figma. Let me know if this should look differently!
Without T&C set:
With T&C set:
How to test the changes in this Pull Request:
Other information: