Primary button, stylesheet restructure by bobheadxi · Pull Request #20 · nwplus/nwhacks2019

bobheadxi · 2018-07-10T08:41:01Z

👷 Changes

Primary button based on figma design
Moved frontendcomponents into demo
Reorganized button styles
Reorganized stylesheet imports
Color declarations in colors.sass - choose from these from now on!

Updated with disabled states

Updated with animated transitions

💭 Notes

For stylesheets, there should only be one import for general styles in index.js:

import React from 'react';
import ReactDOM from 'react-dom';

import App from './components/app/App';
import './styles/index.sass';

ReactDOM.render(<App />, document.getElementById('app'));

All other styles should be imported via styles/index.sass:

/* core */
@import ./colors
@import ./typography

/* components */
@import ../components/index

/* containers */
@import ../containers/index

etc etc.

🔦 Testing Instructions

make web

then go to http://localhost:8080/ui_demo

All stylesheet imports should only go one way - from the primary stylesheet in 'styles/index.sass'

coveralls · 2018-07-10T08:45:25Z

Pull Request Test Coverage Report for Build 246

5 of 10 (50.0%) changed or added relevant lines in 4 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+31.02%) to 31.019%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
web/components/app/App.js	0	1	0.0%
web/components/demo/index.js	0	4	0.0%

Totals
Change from base Build 227:	31.02%
Covered Lines:	48
Relevant Lines:	172

💛 - Coveralls

mingyokim

Quick review for now - will complete review later

mingyokim · 2018-07-12T04:13:57Z

+  <button
+    onClick={onClick}
+    type="button"
+    className={`primary ${disabled ? 'disabled' : ''}`}>


Can we disable buttons by doing <button disabled>Button</button> instead?
https://stackoverflow.com/questions/14750078/style-disabled-button-with-css

That's much cleaner thanks, addressed in 7b2f778

sherryyx

nice transition 🌹

mingyokim

I just noticed that PrimaryButton and SecondaryButton are basically the same except the class name. I think it’d be nice to refactor them into one component, and maybe pass in an additional prop like “type” that specifies whether the button is primary/secondary

bobheadxi · 2018-07-12T06:53:58Z

@mingyokim

refactor them into one component, and maybe pass in an additional prop like “type” that specifies whether the button is primary/secondary

I think that's a pretty bad idea - having them be distinct classes is useful and is in line with how I've seen stateless React components being used (small, simple, occasionally very similar bits and pieces you can use elsewhere). If we pass a button type as a prop, we might as well forgo the React component entirely and just use a bare <button ... /> element when we need buttons, since we're assigning all the same properties anyway.

The more props a component takes, the closer to a normal HTML element we get, and in this case if we add a "type" we're basically back to a bare HTML element, which sort of defeats the whole purpose.

IMO the current implementation is fine, and more convenient to use, without too much repetition. Maybe if we have TertiaryButton, QuaternaryButton, QuinaryButton it might be more prudent to have the type be a prop, but I don't forsee that happening

mingyokim

Some minor feedback: we should probably add some really basic tests to the button component and refactor primary/secondary button into a single component

mingyokim · 2018-07-12T06:53:49Z

+    </div>
+    <br />
+    <div>
+      <PrimaryButton text="Primary" onClick={ButtonCallback} disabled />


I would like to see some specs that ensures onClick callback is not called when it's disabled, and that it's called when it's not disabled

I'll look into adding a few simple tests for this PR, though I'll just point out that the disabled-ness is enforced in the CSS:

&:disabled pointer-events: none

Yeah I get that's enforced in the CSS but we can't predict how the CSS file would change later on - tests help us ensure that our components are still working as expected no matter how much our code changes in the future.

See 941495c

mingyokim · 2018-07-12T06:54:20Z

@@ -0,0 +1,7 @@
+button.primary
+  @include gradient-hover-animation($gradient-primary, $gradient-primary-highlight)


mingyokim · 2018-07-12T06:56:09Z

@@ -0,0 +1,13 @@
+button.secondary
+  background: $gradient-secondary


We should make the styling for SecondaryButton the same as PrimaryButton either in this PR or create a follow-up issue.

OR we could refactor PrimaryButton and SecondaryButton into one component as I've already mentioned

Ok we've decided not to refactor it, but i'd still like to see consistent sass between the two components? Like I'm not seeing the @include gradient-hover-animation($gradient-primary, $gradient-primary-highlight) piece here

What do you mean about the styling? I might agree on making them the same with only colour variations, but the special CSS required to make linear-gradient transition properly sorta makes that more effort than its worth IMO 😞

Like I'm not seeing the @include gradient-hover-animation($gradient-primary, $gradient-primary-highlight) piece here

The secondary button transitions to a flat color $secondary-highlight, which works with -webkit-transition, but the primary button's gradient-to-gradient transition doesn't rip, hence the need for the mixin on one and not the other.

A lot of the shared styles are already in index.sass of buttons, which worked pretty well since they pretty much all applied to the progress group buttons as well (see the CSS in #21)

Thought: we should be careful about making the styling too tightly coupled between the components, because it makes us really inflexible when changes are requested - it's easier to copy styles across multiple components than it is to disentangle a property from tightly coupled styles.

Okay nevermind, i see what you mean

and yeah i'm a noob to css so i don't fully understand the pros/cons of the two approaches but i think if we have a fully fleshed out style guide then it's possible to achieve styles that are tightly coupled between the components

i guess let's bring it up in our channel/next meeting?

mingyokim · 2018-07-12T06:56:59Z

@@ -0,0 +1,7 @@
+import PrimaryButton from './PrimaryButton';


Question about the location of the button component: is button an input?

Absolutely! The standard way to declare buttons used to be <input type="button" />

mingyokim · 2018-07-12T06:58:18Z

+//   $main        linear-gradient to use as normal background
+//   $hover       linear-gradient to transition to upon hover
+//   $transition  [optional] transition time, defaults to $transition-time
+@mixin gradient-hover-animation($main, $hover, $transition: $transition-time )


...OK i trust you

mingyokim · 2018-07-12T07:00:04Z

@@ -0,0 +1,20 @@
+/* colors */
+$white: #FFFFFF


yaaas we need this - we should also consider having a set variables for spacing as well.

@sherryyx is there currently a spacing schema?

(^ not part of this PR) - something to consider going forward

mingyokim · 2018-07-12T07:01:56Z

+@import ./animations
+
+/* components */
+@import ../components/index


💪i like how the order of import is really clear here

mingyokim · 2018-07-12T07:07:41Z

@bobheadxi Sure, we can just leave it as two separate components until we start seeing a lot of repetitions - which as you've mentioned is probably unlikely.

mingyokim

Looks good, ship it! 🚀

Btw @sherryyx added the clicked state for the buttons but let's make that a separate PR

mingyokim · 2018-07-14T18:09:49Z

+    test('should not be clickable', () => {
+      expect(wrapper.find('button').props().disabled).toBeTruthy();
+
+      // Enzyme click simulations do not respect 'disabled' attribute.


mingyokim · 2018-07-14T18:10:00Z

+
+    test('should be clickable', () => {
+      wrapper.find('button').simulate('click');
+      expect(clicked).toBeTruthy();


bobheadxi · 2018-07-14T18:23:48Z

Btw @sherryyx added the clicked state for the buttons but let's make that a separate PR

😍 😍 😍 😍 😍 😍 😍 😍 😍

bobheadxi added 5 commits July 10, 2018 00:28

Create input component group, rename frontendcomponents page to Demo

b9865a4

Add PrimaryButton

d811206

Group shared button styles

e17e169

Define colors and gradients in central colors.sass

e688c40

Restructure stylesheet imports

d8994c5

All stylesheet imports should only go one way - from the primary stylesheet in 'styles/index.sass'

bobheadxi added :web status: wip labels Jul 10, 2018

bobheadxi added 3 commits July 10, 2018 23:08

Merge branch 'master' into web/primary-button

560d12a

Import color classes from Figma

2876767

Use nested classes

c818cc3

bobheadxi requested review from chamkank, mingyokim and sherryyx July 11, 2018 06:38

bobheadxi added status: finalized and removed status: wip labels Jul 11, 2018

Add disabled button states

3614956

bobheadxi force-pushed the web/primary-button branch from 6a4ed2d to 3614956 Compare July 11, 2018 06:41

bobheadxi added 2 commits July 11, 2018 00:21

Button animation

136f664

Abstract linear-gradient animation style into a mixin

890b3e1

bobheadxi force-pushed the web/primary-button branch from fb40068 to 890b3e1 Compare July 11, 2018 07:39

Move shared .disabled traits into index.sass

5416523

bobheadxi mentioned this pull request Jul 11, 2018

Progress button group #21

Merged

mingyokim suggested changes Jul 12, 2018

View reviewed changes

sherryyx approved these changes Jul 12, 2018

View reviewed changes

Use :disabled instead of custom disabled class

7b2f778

mingyokim reviewed Jul 12, 2018

View reviewed changes

mingyokim suggested changes Jul 12, 2018

View reviewed changes

bobheadxi added status: wip and removed status: finalized labels Jul 12, 2018

bobheadxi added 3 commits July 12, 2018 23:13

Add tests for Primary and Secondary button

941495c

Configure Jest test coverage

86f38be

Add combined coverage reporting

291de86

bobheadxi added status: finalized and removed status: wip labels Jul 13, 2018

bobheadxi requested review from chamkank and mingyokim and removed request for chamkank July 13, 2018 06:57

bobheadxi added 2 commits July 13, 2018 00:01

Exclude /tests/ from web coverage

e52873c

Fix travis formatting

1bcd4cf

mingyokim approved these changes Jul 14, 2018

View reviewed changes

bobheadxi merged commit ba8666a into master Jul 14, 2018

bobheadxi deleted the web/primary-button branch August 1, 2018 05:30

		@@ -0,0 +1,7 @@
		button.primary
		@include gradient-hover-animation($gradient-primary, $gradient-primary-highlight)

		@@ -0,0 +1,13 @@
		button.secondary
		background: $gradient-secondary

Conversation

bobheadxi commented Jul 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

👷 Changes

Updated with disabled states

Updated with animated transitions

💭 Notes

🔦 Testing Instructions

Uh oh!

coveralls commented Jul 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 246

💛 - Coveralls

Uh oh!

mingyokim left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sherryyx left a comment

Choose a reason for hiding this comment

Uh oh!

mingyokim left a comment

Choose a reason for hiding this comment

Uh oh!

bobheadxi commented Jul 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mingyokim left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobheadxi Jul 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mingyokim commented Jul 12, 2018

Uh oh!

mingyokim left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobheadxi commented Jul 10, 2018 •

edited

Loading

coveralls commented Jul 10, 2018 •

edited

Loading

bobheadxi commented Jul 12, 2018 •

edited

Loading

bobheadxi Jul 12, 2018 •

edited

Loading