1385 and 1386 button updates #1391

toni-sharpe · 2017-07-04T13:55:46Z

Description

d8ea3eb

Uses display: flex for Button text vertical centring
In preparation for Two tier button text #1385 and Smaller font size allowed on large Buttons #1386 where the text size will become more flexible

e90261b

Allows smaller font to be set in large Buttons

a019c34

Allows two-tier button with subtext

Note: the one pixel difference in internal alignment is because I let flex do it's thing centring rather than adding an extra pixel of pushing around, which I didn't think was a good idea.

TODO

Release notes

Related Issues / Pull Requests

#1385
#1386

Screenshots / Gifs

Left is the design, right is the demo site:

Testing Instructions

Use the demo site: check the subtext prop will add the text as expected, but only if the button size is large

Browsers

QA Issues

flex problems in Edge
flex problems in Safari
smallFont prop being sent through to Link erroneously

* In preparation for #1385 and #1386 where the text size will become more flexible

* Text internal to button is positioned one pixel higher due to allowing flex to centre, rather than using a one pixel offset

adamgeorgeson

Minor doc comments. Out of interest are these output in the demo site?

adamgeorgeson · 2017-07-04T19:15:07Z

src/components/button/button.js

@@ -58,6 +58,21 @@ class Button extends React.Component {
    disabled: PropTypes.bool,

    /**
+     * Allows a font size to be set that alters the default font size.
+     * Currently only setting a smalleer font in a large button is allowed, which we do with CSS


adamgeorgeson · 2017-07-04T19:16:50Z

src/components/button/button.js

+
+    /**
+     * Allows a font size to be set that alters the default font size.
+     * Currently only setting a smalleer font in a large button is allowed, which we do with CSS


Allows subtext to be set. Currently only allowing when button is large.

abramin

The propType functions aren't currently checking for type - i.e. They will allow any values to be entered.
e.g. The smallText prop seems to a Boolean but will not throw an error if a string is passed.

abramin · 2017-07-05T06:35:07Z

src/components/button/button.js

+     * Allows a font size to be set that alters the default font size.
+     * Currently only setting a smalleer font in a large button is allowed, which we do with CSS
+     *
+     * @property fontSize


property name doesn't match

abramin · 2017-07-05T06:36:06Z

src/components/button/button.js

+     * Allows a font size to be set that alters the default font size.
+     * Currently only setting a smalleer font in a large button is allowed, which we do with CSS
+     *
+     * @property fontSize


This documentation doesn't apply to subtext

abramin · 2017-07-05T06:37:40Z

src/components/button/button.js

+     * Currently only setting a smalleer font in a large button is allowed, which we do with CSS
+     *
+     * @property fontSize
+     * @type {String}


Should this be Boolean?

I was wondering this too. The return values made me question myself.

toni-sharpe · 2017-07-05T08:13:32Z

CHANGELOG.md

@@ -171,6 +171,8 @@ return (

 * `Alert` now alerts itself to screen readers.
 * `Browser`: add a new method `setInputFocus` to focus on the input field of passed in ref but does not select text
+* `Button`: Allows small text on a large button [#1386](https://github.com/Sage/carbon/issues/1386)


Move these two lines to v1.3.0 in CHANGELOG.md

abramin · 2017-07-05T08:24:52Z

src/components/button/button.js

+    size: PropTypes.string,
+
+    /**
+     * Sets a second bit of text under the main text, feinter and smaller.


[minor] fainter

They use feint to describe extremely lightly lined paper and I extrapolated from that, however, I might actually be wrong to use the word like this.

Really, I always thought feint meant something like a fake attack.

https://www.collinsdictionary.com/dictionary/english/feint-ruled-paper

I'll change this though I think this is less internationally understandable

* Alphabetise private `Button` functions. * Correct comments. * Check for Boolean type explicitly in `smallFont` proptype function * Ensure nice deafult for `smallFont`

abramin

Thinking more about this, I'm not sure what need there is for the smallFont prop. It seems only relevant when you have a large button and subtext present.

toni-sharpe · 2017-07-10T08:03:37Z

@abramin the smallFont prop is required because when the button is used alongside one with ~~split text~~ two tier text - the issue has more detail

toni-sharpe · 2017-07-10T08:04:18Z

They're actually two separate things, smallFont is not used to govern the font-size of the two tier button

abramin · 2017-07-10T08:15:25Z

I didn't entirely understand the issues. When you say split text do you mean subText?
I see now that the smallFont prop is separate but I'm still not clear about its purpose.
Do we always want to keep font size the same between large and medium buttons perhaps? Maybe worth checking with UX.

toni-sharpe · 2017-07-10T08:24:51Z

@abramin It's separate because I want to be able to control the font-size of one button so that the font can be set to a size which is visually coherent when that button is used with a second button that has two tier text. This prop defaults to false, so the button text will stay large unless purposely chosen.

It's feasible someone could use this prop on it's own, which I didn't think was a terrible thing to offer the developer, it might come out in other designs too, these things often do.

* The small font change relating to issue #1386 was reverted as it was decided to add this font size to all large buttons * This commit also includes the font-size change

… into 1385-and-1386-button-updates

abramin

The CSS change will need to be referenced in the release notes.

abramin · 2017-07-10T11:37:10Z

src/components/button/button.scss

@@ -35,9 +38,20 @@
  }

  &.carbon-button--large {
-    font-size: 16px;
+    font-size: $app-font-size;


could we just not remove this line? As it's set in the parent class,

* Font size setting was not needed on large button * Release note for font size

abramin

The smallFont prop is still in defaultProps.

abramin · 2017-07-12T08:10:30Z

src/components/button/button.scss

  border: $carbon-button__border--secondary;
  border-radius: $carbon-button__border-radius;
  box-sizing: border-box;
  cursor: pointer;
-  display: inline-block;
+  display: inline-flex;


These CSS changes seem to have broken the button contents in Safari.

@abramin Thanks for the cross browser checking!

Could I have some more details on context and steps to reproduce both of the above?

The screenshots are from the demo the site in Safari. To see the hamburger menu you need to shrink the browser a bit.

Right thanks for that, I'll get a look

Looks fine in Chrome & Firefox.

Menu button broken in Edge but regular button looks fine.

Don't have IE11 to test locally.

I've updated the PR description so this can be more easily tracked

* We need to use an internal wrapper when there are multiple children in the button * This is because flex will not apply to certain HTML elements in certain browsers * [link for more](https://stackoverflow.com/questions/35464067/flexbox-not-working-on-button-element-in-safari) * Set the same flex definitions on the internal wrapper so if it exists it applies to those children * `column` is also applied as our children are stacked for this quite tightly controlled contents * Also fix demo site menu spacing

I have QA'd the recent changes 👍

toni-sharpe added Enhancement New Designs Pending QA Pending Review labels Jul 4, 2017

toni-sharpe added this to the v1.2.0 milestone Jul 4, 2017

toni-sharpe self-assigned this Jul 4, 2017

Toni-Leigh Sharpe added 3 commits July 4, 2017 16:11

Uses display: flex for Button text vertical centring

d8ea3eb

* In preparation for #1385 and #1386 where the text size will become more flexible

Allows smaller font to be set in large Buttons #1386

e90261b

Allows two-tier button with subtext #1385

a019c34

* Text internal to button is positioned one pixel higher due to allowing flex to centre, rather than using a one pixel offset

toni-sharpe force-pushed the 1385-and-1386-button-updates branch from e20f135 to a019c34 Compare July 4, 2017 15:14

This was referenced Jul 4, 2017

Smaller font size allowed on large Buttons #1386

Closed

Two tier button text #1385

Closed

andrewjtait modified the milestones: v1.3.0, v1.2.0 Jul 4, 2017

adamgeorgeson suggested changes Jul 4, 2017

View reviewed changes

taoliusage approved these changes Jul 4, 2017

View reviewed changes

abramin suggested changes Jul 5, 2017

View reviewed changes

toni-sharpe commented Jul 5, 2017

View reviewed changes

toni-sharpe force-pushed the 1385-and-1386-button-updates branch from 046eeb9 to 4b450b6 Compare July 5, 2017 08:20

abramin reviewed Jul 5, 2017

View reviewed changes

abramin approved these changes Jul 5, 2017

View reviewed changes

toni-sharpe force-pushed the 1385-and-1386-button-updates branch from 38edfd1 to 3e8e091 Compare July 5, 2017 08:49

adamgeorgeson approved these changes Jul 5, 2017

View reviewed changes

toni-sharpe added Passed Review (2/2) and removed Pending Review labels Jul 5, 2017

toni-sharpe force-pushed the 1385-and-1386-button-updates branch from 3e8e091 to eee624b Compare July 5, 2017 08:55

Toni-Leigh Sharpe added 3 commits July 5, 2017 09:55

PR Comments #1385 & #1386

bc7c679

* Alphabetise private `Button` functions. * Correct comments. * Check for Boolean type explicitly in `smallFont` proptype function * Ensure nice deafult for `smallFont`

Move release notes to v1.3.0 #1385 #1386

eee624b

Merge branch 'master' into 1385-and-1386-button-updates

3a8286f

toni-sharpe mentioned this pull request Jul 5, 2017

Rewrite button specs using snapshots and enzyme #1399

Closed

1 task

abramin suggested changes Jul 10, 2017

View reviewed changes

Toni Leigh Sharpe added 5 commits July 10, 2017 09:27

Merge branch 'master' into 1385-and-1386-button-updates

347c51f

Reverts smallFont change

7f2d0b8

* The small font change relating to issue #1386 was reverted as it was decided to add this font size to all large buttons * This commit also includes the font-size change

Merge branch '1385-and-1386-button-updates' of github.com:Sage/carbon…

91e822b

… into 1385-and-1386-button-updates

Removes erroneous release note #1386

692b060

Missed merge error #1386

dc53d2a

abramin suggested changes Jul 10, 2017

View reviewed changes

PR Comments #1385

354a729

* Font size setting was not needed on large button * Release note for font size

abramin approved these changes Jul 10, 2017

View reviewed changes

abramin suggested changes Jul 12, 2017

View reviewed changes

abramin previously requested changes Jul 12, 2017

View reviewed changes

andrewjtait added Failed QA Pending QA and removed Pending QA labels Jul 20, 2017

andrewjtait modified the milestones: v1.4.0, v1.3.0 Jul 21, 2017

Toni Leigh Sharpe added 3 commits July 24, 2017 09:24

Merge branch 'master' into 1385-and-1386-button-updates

f044254

Remove un-needed prop #1385

10386b2

toni-sharpe removed the Failed QA label Jul 24, 2017

andrewjtait added Passed QA and removed Pending QA labels Jul 25, 2017

Merge branch 'master' into 1385-and-1386-button-updates

ac1797a

andrewjtait merged commit 36cf5a2 into master Jul 25, 2017

andrewjtait deleted the 1385-and-1386-button-updates branch July 25, 2017 09:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1385 and 1386 button updates #1391

1385 and 1386 button updates #1391

toni-sharpe commented Jul 4, 2017 •

edited

Loading

adamgeorgeson left a comment

adamgeorgeson Jul 4, 2017

adamgeorgeson Jul 4, 2017

abramin left a comment

abramin Jul 5, 2017

abramin Jul 5, 2017

abramin Jul 5, 2017

adamgeorgeson Jul 5, 2017

toni-sharpe Jul 5, 2017

abramin Jul 5, 2017

toni-sharpe Jul 5, 2017

abramin Jul 5, 2017

toni-sharpe Jul 5, 2017

abramin left a comment

toni-sharpe commented Jul 10, 2017 •

edited

Loading

toni-sharpe commented Jul 10, 2017

abramin commented Jul 10, 2017 •

edited

Loading

toni-sharpe commented Jul 10, 2017

abramin left a comment

abramin Jul 10, 2017

abramin left a comment •

edited

Loading

abramin Jul 12, 2017

toni-sharpe Jul 12, 2017

abramin Jul 12, 2017

toni-sharpe Jul 12, 2017

abramin Jul 12, 2017

toni-sharpe Jul 12, 2017

1385 and 1386 button updates #1391

1385 and 1386 button updates #1391

Conversation

toni-sharpe commented Jul 4, 2017 • edited Loading

Description

TODO

Related Issues / Pull Requests

Screenshots / Gifs

Testing Instructions

Browsers

QA Issues

adamgeorgeson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abramin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abramin left a comment

Choose a reason for hiding this comment

toni-sharpe commented Jul 10, 2017 • edited Loading

toni-sharpe commented Jul 10, 2017

abramin commented Jul 10, 2017 • edited Loading

toni-sharpe commented Jul 10, 2017

abramin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abramin left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toni-sharpe commented Jul 4, 2017 •

edited

Loading

toni-sharpe commented Jul 10, 2017 •

edited

Loading

abramin commented Jul 10, 2017 •

edited

Loading

abramin left a comment •

edited

Loading