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
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to鈥
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -10,12 +10,13 @@ notifications:

before_script:
- make deps
- yarn global add nyc
- yarn global add coveralls

script:
- make lint # static analysis
- make build # attempt to compile web app
- make test # run all tests

after_success:
# only report serverless coverage for now
- (cd ./serverless ; npm run coverage)
- make report-coverage
@@ -19,6 +19,13 @@ lint:
(cd ./serverless ; yarn lint)
(cd ./web ; yarn lint)

.PHONY: report-coverage
report-coverage:
mkdir -p .nyc_output
cp serverless/coverage/coverage-final.json .nyc_output/coverage-serverless.json
cp web/coverage/coverage-final.json .nyc_output/coverage-web.json
nyc report --reporter=text-lcov | coveralls

###############################
# Component-specific commands #
###############################
@@ -6,10 +6,19 @@
"license": "MIT",
"scripts": {
"start": "echo 'google cloudfunctions doesn't suppor serverless-offline, use 'serverless invoke local --function example'",
"test": "nyc jest",
"coverage": "nyc report --reporter=text-lcov | coveralls",
"test": "jest",
"lint": "eslint . --ignore-pattern public/"
},
"jest": {
"collectCoverage": true,
"collectCoverageFrom" : [
"src/**/*.js",
"!**/node_modules/**",
"!**/*.test.js",
"!**/coverage/**"
],
"coverageReporters": ["text", "json"]
},
"dependencies": {
"got": "8.3.1",
"serverless": "1.27.3",
@@ -24,13 +33,5 @@
"jest": "^23.1.0",
"nock": "^9.3.3",
"nyc": "^12.0.2"
},
"nyc": {
"exclude": [
"**/node_modules/**",
"**/tests/**",
"**/coverage/**"
],
"all": true
}
}
@@ -10,9 +10,7 @@ import AdminPanel from '../admin';
import NotFound from '../errors/NotFound';
import Navbar from '../../containers/navbar';
import DashBoard from '../dashboard';
import FrontEndComponent from '../frontEndComponents';

import './App.sass';
import UIDemo from '../demo';

import configureStore from '../../services/store';

@@ -32,7 +30,7 @@ const App = () => (
<Route path="/logout" component={Logout} />
<Route path="/dashboard" component={DashBoard} />
<Route path="/admin" component={AdminPanel} />
<Route path="/front_end_components" component={FrontEndComponent} />
<Route path="/ui_demo" component={UIDemo} />
<Route path="/page_not_found" component={NotFound} />
<Route path="*" component={() => <Redirect to="/page_not_found" />} />
</Switch>
@@ -0,0 +1,25 @@
import React from 'react';

import { SecondaryButton, PrimaryButton } from '../input/buttons';

const ButtonCallback = e => console.log(`${e.currentTarget.textContent} button clicked!`);

const FrontEndComponents = () => (
<div>
<p>Buttons</p>
<br />
<div>
<PrimaryButton text="Primary" onClick={ButtonCallback} />
&nbsp;
<SecondaryButton text="Secondary" onClick={ButtonCallback} />
</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

&nbsp;
<SecondaryButton text="Secondary" onClick={ButtonCallback} disabled />
</div>
</div>
);

export default FrontEndComponents;

This file was deleted.

This file was deleted.

This file was deleted.

@@ -0,0 +1,4 @@
// Import all component styles here
@import ./app/App
@import ./navbar/Navbar
@import ./input/buttons/index
@@ -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

馃帀

border: none
color: $white

&:disabled
background: $gradient-primary-subdued
@@ -0,0 +1,59 @@
import React from 'react';
import { shallow, configure } from 'enzyme';
import Adapter from 'enzyme-adapter-react-16';

import PrimaryButton from '.';

beforeAll(() => {
configure({ adapter: new Adapter() });
});

describe('PrimaryButton component', () => {
let clicked;
let wrapper;
const getWrapper = props => shallow(<PrimaryButton {...props} />);

describe('when the button is enabled', () => {
beforeEach(() => {
clicked = false;
wrapper = getWrapper({
text: 'hello cham',
onClick: () => { clicked = true; },
disabled: false,
});
});

test('should have text', () => {
expect(wrapper.text()).toEqual('hello cham');
});

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

馃帀

});
});

describe('when the button is disabled', () => {
beforeEach(() => {
clicked = false;
wrapper = getWrapper({
text: 'hello cham',
onClick: () => { clicked = true; },
disabled: true,
});
});

test('should have text', () => {
expect(wrapper.text()).toEqual('hello cham');
});

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

馃槥

// See https://github.com/airbnb/enzyme/issues/386
// wrapper.find('button').simulate('click');
// expect(clicked).toBeFalsy();
});
});
});
@@ -0,0 +1,20 @@
import React from 'react';
import PropTypes from 'prop-types';

const PrimaryButton = ({ text, onClick, disabled }) => (
<button
onClick={onClick}
type="button"
disabled={disabled}
className="primary">
{ text }
</button>
);

PrimaryButton.propTypes = {
text: PropTypes.string.isRequired,
onClick: PropTypes.func,
disabled: PropTypes.bool,
};

export default PrimaryButton;
@@ -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

馃殌

border: 1px solid $secondary
color: $secondary

&:hover
border: 1px solid $secondary-highlight
color: $secondary-highlight

&:focus
/* Link text/Hover */
border: 1px solid $secondary-highlight
color: $secondary-highlight
@@ -0,0 +1,59 @@
import React from 'react';
import { shallow, configure } from 'enzyme';
import Adapter from 'enzyme-adapter-react-16';

import SecondaryButton from '.';

beforeAll(() => {
configure({ adapter: new Adapter() });
});

describe('SecondaryButton component', () => {
let clicked;
let wrapper;
const getWrapper = props => shallow(<SecondaryButton {...props} />);

describe('when the button is enabled', () => {
beforeEach(() => {
clicked = false;
wrapper = getWrapper({
text: 'hello cham',
onClick: () => { clicked = true; },
disabled: false,
});
});

test('should have text', () => {
expect(wrapper.text()).toEqual('hello cham');
});

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

describe('when the button is disabled', () => {
beforeEach(() => {
clicked = false;
wrapper = getWrapper({
text: 'hello cham',
onClick: () => { clicked = true; },
disabled: true,
});
});

test('should have text', () => {
expect(wrapper.text()).toEqual('hello cham');
});

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

// Enzyme click simulations do not respect 'disabled' attribute.
// See https://github.com/airbnb/enzyme/issues/386
// wrapper.find('button').simulate('click');
// expect(clicked).toBeFalsy();
});
});
});
@@ -1,17 +1,20 @@
import React from 'react';
import PropTypes from 'prop-types';

import './SecondaryButton.sass';

const SecondaryButton = ({ text, onClick }) => (
<button onClick={onClick} type="button" className="secondary">
const SecondaryButton = ({ text, onClick, disabled }) => (
<button
onClick={onClick}
type="button"
disabled={disabled}
className="secondary">
{ text }
</button>
);

SecondaryButton.propTypes = {
text: PropTypes.string.isRequired,
onClick: PropTypes.func,
disabled: PropTypes.bool,
};

export default SecondaryButton;
@@ -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
import SecondaryButton from './SecondaryButton';

export {
PrimaryButton,
SecondaryButton,
};
ProTip! Use n and p to navigate between commits in a pull request.
You can鈥檛 perform that action at this time.