Skip to content

LG-12103 Embed Styles into Login Button Lookbook Component#10387

Merged
nprimak merged 18 commits intomainfrom
np/Lg-12103
Apr 11, 2024
Merged

LG-12103 Embed Styles into Login Button Lookbook Component#10387
nprimak merged 18 commits intomainfrom
np/Lg-12103

Conversation

@nprimak
Copy link
Contributor

@nprimak nprimak commented Apr 9, 2024

🎫 Ticket

Link to the relevant ticket:
LG-12103

Original login button PR for context: #9840

🛠 Summary of changes

The login sign up button is embedded into our dev docs for easy use for our partners. We were not displaying the HTML panel for the login button in the developer docs because it was missing the styles that go along with the button. This ticket adds those styles so partners can more easily utilize the button "out of the box" and not have to write as much CSS.

With this addition we will be able to update the embed code to include the HTML panel.

@nprimak nprimak requested review from aduth and ajfarkas April 9, 2024 16:33
@aduth
Copy link
Contributor

aduth commented Apr 9, 2024

One thing I hadn't thought to consider previously is if the inline styles will cause any issues for the content-security policy in deployed environments. I think I recall something about the CSP being disabled for the Lookbook component previews. And maybe at worst the inline styles aren't loaded but at least the external CSS file reference will still load and apply its styles.

@aduth
Copy link
Contributor

aduth commented Apr 9, 2024

The "Login.gov" logo looks a little off-center in my testing?

image

@nprimak
Copy link
Contributor Author

nprimak commented Apr 9, 2024

The "Login.gov" logo looks a little off-center in my testing?

image

I tweaked the margins a tiny bit - seemed like it just needed to be nudged down?

@nprimak
Copy link
Contributor Author

nprimak commented Apr 9, 2024

Screenshot 2024-04-09 at 5 32 18 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

Following from my previous comment, if we applied login-button__logo to the SVG we wouldn't need this, because SVGs are display: inline by default.

I'd also be curious to double-check if the vertical-align is actually doing anything for the styling. It doesn't seem to make any impact for me in my testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seemed to shift the logo down when I added vertical-align to the div. I originally had the class on the svg but vertical-align didn't seem to work that way. I'll take another look though, maybe something was in a funky state. I'd also prefer not to have the extra div so if that is unnecessary I will definitely remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aduth yep you're right I was able to remove vertical-align, margin-top, and display-inline and I moved the class to the svg

nprimak and others added 14 commits April 10, 2024 12:17
changelog: User Facing Improvements, Login Design System Component, login-button embed styles foor partner use
Co-authored-by: Andrew Duthie <1779930+aduth@users.noreply.github.com>
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
Co-authored-by: Andrew Duthie <1779930+aduth@users.noreply.github.com>
nprimak and others added 2 commits April 10, 2024 16:00
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
@nprimak
Copy link
Contributor Author

nprimak commented Apr 10, 2024

Tweaked the logo sizes a bit

Screenshot 2024-04-10 at 4 04 13 PM
Screenshot 2024-04-10 at 4 04 06 PM


.login-button.usa-button--big > .login-button__logo {
font-size: 1.8rem;
margin-top: -2px;
Copy link
Contributor

Choose a reason for hiding this comment

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

praise: hooray for cleanup!

I always like to see negative margins get deprecated :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yay!

Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Looks good after the one suggestion about @after_render. Nice simplification!

Co-authored-by: Andrew Duthie <1779930+aduth@users.noreply.github.com>
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.

5 participants