Skip to content

login button component#9840

Merged
nprimak merged 20 commits intomainfrom
np/LG-11902/login-button-component
Jan 11, 2024
Merged

login button component#9840
nprimak merged 20 commits intomainfrom
np/LG-11902/login-button-component

Conversation

@nprimak
Copy link
Contributor

@nprimak nprimak commented Jan 2, 2024

🎫 Ticket

LG-11902

🛠 Summary of changes

Creating a login button component where the workbench preview will be embedded into the agency logo page for the dev docs for LG-11728

Refer to the Figma design here: https://www.figma.com/file/Vii2kxbapRsAXr3M8K2Ecy/UX-Guide%3A-Sign-in-button-examples-and-images-(LG-10129)?type=design&node-id=69-1626&mode=design&t=3jmQmschHLz26SXP-0

@nprimak nprimak requested a review from aduth January 2, 2024 15:03
@nprimak nprimak marked this pull request as ready for review January 3, 2024 19:57
Copy link
Contributor

Choose a reason for hiding this comment

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

do we typically indent by 2 in CSS like we do for our other files? (.js, .rb)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

surprised the linter didn't fix it - yes I will fix!

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like our linter isn't checking these component stylesheets. I can help put together a fix for that

"lint:css": "stylelint 'app/assets/stylesheets/**/*.scss' 'app/javascript/**/*.scss'",

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I follow if / how this class is extended to be a subclass of ButtonComponent, where if it were a subclass, I'd expect we wouldn't duplicate usa-button here, and we'd be deferring the rendering to ButtonComponent in some way.

I might suggest avoiding making this a subclass. If we wanted, we could reuse behaviors of ButtonComponent by rendering it in the template: render ButtonComponent.new(...) instead of content_tag(:button, ...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah the only thing I was extending was making the button get bigger but I easily remedied that by just adding

classes << 'usa-button--big' if big

on line 25

Copy link
Contributor

Choose a reason for hiding this comment

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

Somewhat related to the prior point about subclassing: color is not a property of ButtonComponent, so I wouldn't expect we'd be passing it this way. Currently, it's being passed through as an attribute of the button tag, which is unexpected:

<button color="primary-darker" class="usa-button login-button login-button--primary-darker"> 
  Sign in with <span class="login-button__logo login-button__logo-white">Login.gov</span>
</button>

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.

I think this'll be all set after this batch of comments 👍

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.

LGTM after the lint issue is resolved.

nprimak and others added 19 commits January 10, 2024 14:06
Co-authored-by: Andrew Duthie <1779930+aduth@users.noreply.github.com>
changelog: Upcoming Features, Lookbook Component, Login button component for dev docs (LG-11902)
Co-authored-by: Andrew Duthie <1779930+aduth@users.noreply.github.com>
Co-authored-by: Andrew Duthie <1779930+aduth@users.noreply.github.com>
Co-authored-by: Andrew Duthie <1779930+aduth@users.noreply.github.com>
Co-authored-by: Andrew Duthie <1779930+aduth@users.noreply.github.com>
Co-authored-by: Andrew Duthie <1779930+aduth@users.noreply.github.com>
Co-authored-by: Andrew Duthie <1779930+aduth@users.noreply.github.com>
@nprimak nprimak force-pushed the np/LG-11902/login-button-component branch from 92c0783 to 3692e6e Compare January 10, 2024 19:33
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.

LGTM!

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.

3 participants