Skip to content

Added Button level 'text' and added yalc note to README.md#133

Merged
pixelbandito merged 2 commits intomasterfrom
chore/add-button-level-text
Jan 2, 2020
Merged

Added Button level 'text' and added yalc note to README.md#133
pixelbandito merged 2 commits intomasterfrom
chore/add-button-level-text

Conversation

@pixelbandito
Copy link
Copy Markdown
Contributor

Added .yalc instructions with a big warning to the README.

Added a "Text" level to button that looks like blue text, no border. I did this because:

  1. I noticed Invision had a button like that in the stylesheet.
  2. Bootstrap and Foundation both have buttons like that, but theirs are called "Link"s, both preserve the padding of normal buttons, and one has an underline on hover.
  3. In Zeplin, Egle had two borderless buttons, one called "Text/Blue" and one called "Text/Gray", and without padding.

I figured "text" was a little less confusing than "link" as a modifier, since these don't get typical underline styles.
I also figured I'd leave the padding / sizing as it is, following Bootstrap/Foundation, and ppl can override the styles with padding: 0 when/if necessary.
I don't know what to do as far as naming convention to add the gray and blue variations. (Request for comments)

Copy link
Copy Markdown
Contributor

@chelshaw chelshaw 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, thanks for the yalc instructions

Copy link
Copy Markdown
Contributor

@kcj002 kcj002 left a comment

Choose a reason for hiding this comment

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

🌹

Copy link
Copy Markdown
Contributor

@mdespuits mdespuits left a comment

Choose a reason for hiding this comment

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

Looking good. Had a few suggestions but nothing blocking.

Comment thread src/components/Button/style.css Outdated
.text,
.text:link,
.text:visited {
color: var(--rvr-color-text);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Instead of re-declaring the properties and the values, you should be able to declare a component variable that you can re-assign in the scope. For example:

.text {
  --rvr-btn-color: var(--rvr-color-text);
  color: var(--rvr-btn-color); /* sets the default */
}

.text:active {
  /* Changes the variable for the scope, but no property declaration necessary */
  --rvr-btn-color: var(--rvr-color-text-active);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also, this is not a blocker, but more of a bikeshed thought about how to build out these variants

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I like this idea, but without spending some time trying to dedupe styles, it doesn't seem like an obvious improvement in this component (since the variables-to-style rules ratio is 1:1).

I'll think about approaches to drying this up, and If I'm missing something, please let me know. For now, I'd like to merge as-is.

Comment thread src/components/Button/index.js Outdated
'secondary',
'success',
'teal',
'teal',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Double teal

@pixelbandito pixelbandito force-pushed the chore/add-button-level-text branch from 61dec5b to 5d9c96a Compare November 25, 2019 17:35
@pixelbandito pixelbandito force-pushed the chore/add-button-level-text branch 2 times, most recently from b346f3d to 9536350 Compare January 2, 2020 20:49
Copy link
Copy Markdown
Contributor

@mdespuits mdespuits left a comment

Choose a reason for hiding this comment

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

🍔🌹

@pixelbandito pixelbandito force-pushed the chore/add-button-level-text branch from 9536350 to c051196 Compare January 2, 2020 22:31
@pixelbandito pixelbandito merged commit 952301d into master Jan 2, 2020
@mdespuits mdespuits deleted the chore/add-button-level-text branch April 23, 2020 03:14
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.

4 participants