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

feat(button): Implement stroked button #1194

Merged
merged 4 commits into from
Aug 30, 2017

Conversation

yeelan0319
Copy link
Contributor

closes #987

@yeelan0319 yeelan0319 added this to the Button Improvement milestone Aug 27, 2017
Copy link
Contributor

@kfranqueiro kfranqueiro left a comment

Choose a reason for hiding this comment

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

Discussed offline, but for the record: I noticed that in the demo page, the addition of border actually ends up offsetting the text, and as you add to the border-width, it moves the text in the button down, but doesn't move the bottom border, causing the text to become increasingly off-center vertically. Removing the explicit line-height currently set on buttons seems to resolve this, except when applied to <a> elements which end up with the text aligned to the top. This might be worth more investigation.

.mdc-button--stroked {
@include mdc-theme-prop(border-color, text-primary-on-light);

border-width: 1px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure this is the correct width? Images I've seen so far make it look more like 2px.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Confirmed with designer that we set the default border to be 2dp instead of 1dp.

@@ -138,6 +146,7 @@ The provided modifiers are:
| --------------------- | ------------------------------------------------------- |
| `mdc-button--raised` | A contained button that is elevated upon the surface. |
| `mdc-button--unelevated` | A contained button that is flush with the surface. |
| `mdc-button--stroked` | A contained button contained by a rectangular shape. |
Copy link
Contributor

Choose a reason for hiding this comment

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

Re-wording suggestion: A contained button that is flush with the surface and has a visible border.

@kfranqueiro kfranqueiro self-assigned this Aug 28, 2017
@yeelan0319
Copy link
Contributor Author

Chatted offline. For record keeping:

Reason:
border is not considered as part of available space to line-height, so specifying line-height on stroked button will make text aligned border-width off from the top.

Solution goes for:
Dynamically calculate the correct line-height for stroked button.
In the button mixin, we will use base-line-height - 2 * border-width to calculate the correct line-height for stoked button. Note: dense button need to be tackle as well since it have a specification in its line-height.

Solution considered:

  1. Removing line-height: This method works for all button type but failed on link. It seems link button will create a different baseline to align with when there is no line-height specified.
  2. Change to use flexbox: Unfortunately, Safari won't support justify-content to align text horizontally centered

@kfranqueiro Thanks for catching it!

@yeelan0319 yeelan0319 force-pushed the feat/implement-stroked-button branch from b711617 to adadf9c Compare August 28, 2017 20:08
@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@yeelan0319 yeelan0319 changed the base branch from feat/button-improvement to master August 28, 2017 20:08
@googlebot
Copy link

CLAs look good, thanks!

@yeelan0319 yeelan0319 force-pushed the feat/implement-stroked-button branch 2 times, most recently from 929f222 to 2e726e0 Compare August 29, 2017 15:54
@yeelan0319 yeelan0319 force-pushed the feat/implement-stroked-button branch from 2e726e0 to a581d0e Compare August 30, 2017 06:03
@codecov-io
Copy link

codecov-io commented Aug 30, 2017

Codecov Report

Merging #1194 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1194   +/-   ##
======================================
  Coverage    99.9%   99.9%           
======================================
  Files          69      69           
  Lines        3314    3314           
  Branches      406     406           
======================================
  Hits         3311    3311           
  Misses          3       3

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a8b9099...a581d0e. Read the comment docs.

@yeelan0319 yeelan0319 merged commit 56bf37d into master Aug 30, 2017
@yeelan0319 yeelan0319 deleted the feat/implement-stroked-button branch August 30, 2017 06:54
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.

Create new button style: stroked button
4 participants