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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Primary button, stylesheet restructure #20

Merged
merged 18 commits into from Jul 14, 2018

Conversation

@bobheadxi
Copy link
Contributor

bobheadxi commented Jul 10, 2018

馃懛 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

image

Updated with animated transitions

buttonanimatation

馃挱 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

bobheadxi added 5 commits Jul 10, 2018
All stylesheet imports should only go one way - from the primary stylesheet in 'styles/index.sass'
@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 10, 2018

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 Coverage Status
Change from base Build 227: 31.02%
Covered Lines: 48
Relevant Lines: 172

馃挍 - Coveralls
@bobheadxi bobheadxi force-pushed the web/primary-button branch from 6a4ed2d to 3614956 Jul 11, 2018
@bobheadxi bobheadxi force-pushed the web/primary-button branch from fb40068 to 890b3e1 Jul 11, 2018
@bobheadxi bobheadxi mentioned this pull request Jul 11, 2018
Copy link
Contributor

mingyokim left a comment

Quick review for now - will complete review later

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

This comment has been minimized.

Copy link
@mingyokim

mingyokim Jul 12, 2018

Contributor

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

This comment has been minimized.

Copy link
@bobheadxi

bobheadxi Jul 12, 2018

Author Contributor

That's much cleaner thanks, addressed in 7b2f778

Copy link

sherryyx left a comment

nice transition 馃尮

Copy link
Contributor

mingyokim left a comment

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

@bobheadxi

This comment has been minimized.

Copy link
Contributor Author

bobheadxi commented Jul 12, 2018

@mingyokim

refactor them into one component, and maybe pass in an additional prop like 鈥渢ype鈥 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

Copy link
Contributor

mingyokim left a comment

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

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

This comment has been minimized.

Copy link
@mingyokim

mingyokim Jul 12, 2018

Contributor

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

This comment has been minimized.

Copy link
@bobheadxi

bobheadxi Jul 12, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@mingyokim

mingyokim Jul 12, 2018

Contributor

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.

This comment has been minimized.

Copy link
@bobheadxi

bobheadxi Jul 13, 2018

Author Contributor

See 941495c

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

This comment has been minimized.

Copy link
@mingyokim

mingyokim Jul 12, 2018

Contributor

馃帀

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

This comment has been minimized.

Copy link
@mingyokim

mingyokim Jul 12, 2018

Contributor

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

This comment has been minimized.

Copy link
@mingyokim

mingyokim Jul 12, 2018

Contributor

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

This comment has been minimized.

Copy link
@bobheadxi

bobheadxi Jul 12, 2018

Author Contributor

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 馃槥

This comment has been minimized.

Copy link
@bobheadxi

bobheadxi Jul 12, 2018

Author Contributor

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.

This comment has been minimized.

Copy link
@bobheadxi

bobheadxi Jul 12, 2018

Author Contributor

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)

This comment has been minimized.

Copy link
@bobheadxi

bobheadxi Jul 12, 2018

Author Contributor

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.

This comment has been minimized.

Copy link
@mingyokim

mingyokim Jul 12, 2018

Contributor

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?

This comment has been minimized.

Copy link
@bobheadxi

bobheadxi Jul 12, 2018

Author Contributor

馃殌

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

This comment has been minimized.

Copy link
@mingyokim

mingyokim Jul 12, 2018

Contributor

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

This comment has been minimized.

Copy link
@bobheadxi
// $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 )

This comment has been minimized.

Copy link
@mingyokim

mingyokim Jul 12, 2018

Contributor

...OK i trust you

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

This comment has been minimized.

Copy link
@mingyokim

mingyokim Jul 12, 2018

Contributor

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

@sherryyx is there currently a spacing schema?

This comment has been minimized.

Copy link
@mingyokim

mingyokim Jul 12, 2018

Contributor

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

@import ./animations

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

This comment has been minimized.

Copy link
@mingyokim

mingyokim Jul 12, 2018

Contributor

馃挭i like how the order of import is really clear here

@mingyokim

This comment has been minimized.

Copy link
Contributor

mingyokim commented Jul 12, 2018

@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.

@bobheadxi bobheadxi requested review from mingyokim and chamkank and removed request for chamkank Jul 13, 2018
Copy link
Contributor

mingyokim left a comment

Looks good, ship it! 馃殌

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

test('should not be clickable', () => {
expect(wrapper.find('button').props().disabled).toBeTruthy();

// Enzyme click simulations do not respect 'disabled' attribute.

This comment has been minimized.

Copy link
@mingyokim

mingyokim Jul 14, 2018

Contributor

馃槥


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

This comment has been minimized.

Copy link
@mingyokim

mingyokim Jul 14, 2018

Contributor

馃帀

@bobheadxi

This comment has been minimized.

Copy link
Contributor Author

bobheadxi commented Jul 14, 2018

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

馃槏 馃槏 馃槏 馃槏 馃槏 馃槏 馃槏 馃槏 馃槏

@bobheadxi bobheadxi merged commit ba8666a into master Jul 14, 2018
5 checks passed
5 checks passed
Travis CI - Branch Build Passed
Details
Travis CI - Pull Request Build Passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+31.02%) to 31.019%
Details
@bobheadxi bobheadxi deleted the web/primary-button branch Aug 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can鈥檛 perform that action at this time.