Skip to content
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: correct button heights add new default icon size #186

Merged
merged 4 commits into from Nov 15, 2019

Conversation

@SiTaggart
Copy link
Collaborator

SiTaggart commented Nov 15, 2019

  • Correct the button heights by dropping the line-height by one
  • Introduce a new icon size so the default icon makes a button the correct height
@SiTaggart SiTaggart requested a review from TheSisb Nov 15, 2019
@now

This comment has been minimized.

Copy link

now bot commented Nov 15, 2019

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/twilio-dsys/paste/da0iivik3
🌍 Preview: https://paste-git-fix-buttonheights.twilio-dsys.now.sh

SiTaggart added 2 commits Nov 15, 2019
Correct the button heights by dropping the line-height by one
Introduce a new icon size so the default icon makes a button the correct height
@@ -46,6 +45,7 @@ const baseButtonWrapper = (props: ButtonWrapperProps): SerializedStyles => css`
background: none;
transition: background-color 100ms ease-in, border-color 100ms ease-in;
font-family: ${themeGet('fonts.fontFamilyText')(props)};
font-weight: ${themeGet('fontWeights.fontWeightSemibold')(props)};

This comment has been minimized.

Copy link
@SiTaggart

SiTaggart Nov 15, 2019

Author Collaborator
@@ -6,7 +6,7 @@ import {PlusIcon} from '@twilio-paste/icons/esm/PlusIcon';
import {Button} from '../src';
import {ButtonVariants, ButtonSizes, ButtonTabIndexes} from '../src/types';

const ButtonSizeOptions = ['default', 'small', 'icon', 'reset'];
const ButtonSizeOptions = ['', 'default', 'small', 'icon', 'reset'];

This comment has been minimized.

Copy link
@SiTaggart

SiTaggart Nov 15, 2019

Author Collaborator

For non-required props its useful to have stories that default to blank. For example, link buttons default to reset size, but we were setting a size in storybook with knobs.

Setting a size set the font weight to semibold. Reset size was normal.

We didn't have enough coverage for all the options so we didn't catch it. I've added an example of each below for applitools to check going forward

@@ -49,8 +49,11 @@ props:
value: "{!size-square-30}"
comment: Icon sizing token
size-icon-20:
value: "{!size-square-50}"
value: "{!size-square-40}"

This comment has been minimized.

Copy link
@SiTaggart

SiTaggart Nov 15, 2019

Author Collaborator

Shifting the icon size scale. New default is 20px, shift the last 2 up one in the scale

@SiTaggart SiTaggart merged commit 2e54478 into master Nov 15, 2019
8 checks passed
8 checks passed
Semantic Pull Request ready to be squashed
Details
ci/circleci: applitools Your tests passed on CircleCI!
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: prettier Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
now Deployment has completed
Details
scm/applitools No baseline conflicts found! (7 changes found)
Details
tests/applitools No visual tests ran, see "Details" for help
Details
@SiTaggart SiTaggart deleted the fix/button/heights branch Nov 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.