Skip to content

LG-5629 account buttons part 2#5945

Merged
stevegsa merged 22 commits intomainfrom
stevegsa-account-buttons2
Feb 24, 2022
Merged

LG-5629 account buttons part 2#5945
stevegsa merged 22 commits intomainfrom
stevegsa-account-buttons2

Conversation

@stevegsa
Copy link
Contributor

@stevegsa stevegsa commented Feb 14, 2022

Screen Shot 2022-02-14 at 11 51 49 AM

@stevegsa
Copy link
Contributor Author

Hiya @aduth I appreciate the work that's been done with ButtonComponent. I have a few suggestions for improvement:

  1. a simpler way to specify a path vs using action
  2. remove the need for with_content and make it a required param
  3. make a helper method so we don't need the render/new

@stevegsa stevegsa marked this pull request as ready for review February 16, 2022 22:12
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

@aduth
Copy link
Contributor

aduth commented Feb 17, 2022

  1. a simpler way to specify a path vs using action

Yeah, based on how common it is to use a button as a link, I think a link or href option could make sense 👍

Related: #5910 (comment)

@mitchellhenke mitchellhenke force-pushed the stevegsa-account-buttons2 branch from d891124 to c811e8d Compare February 17, 2022 20:46
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 we should be able to remove all the account-action-button styles now as well, since we're removing the last of them.

.account-action-button {
@include u-radius('lg');
background-color: color('primary-lighter');
border: 0;
color: $blue;
display: inline-block;
font-size: 0.8125rem;
font-weight: normal;
line-height: 1.375rem;
margin-bottom: -3px;
margin-top: -3px;
padding: 0.5rem;
padding-bottom: 0.125rem;
padding-top: 0.125rem;
text-decoration: none;
vertical-align: middle;
white-space: normal;
&,
&:hover {
text-decoration: none;
}
}

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 👍

@stevegsa stevegsa merged commit 174f3b3 into main Feb 24, 2022
@stevegsa stevegsa deleted the stevegsa-account-buttons2 branch February 24, 2022 18:28
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