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 12 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,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,19 @@
import React from 'react';
import PropTypes from 'prop-types';

const PrimaryButton = ({ text, onClick, disabled }) => (
<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

{ 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
@@ -1,17 +1,19 @@
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"
className={`secondary ${disabled ? 'disabled' : ''}`}>
{ 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,
};
@@ -0,0 +1,35 @@
@import ./PrimaryButton/PrimaryButton
@import ./SecondaryButton/SecondaryButton

button
cursor: pointer
-webkit-transition: $transition-time

/* box */
border-radius: 4px
box-sizing: border-box
min-width: 80px
height: 40px

/* content */
padding-left: 16px
padding-right: 16px
font-family: Apercu Pro
font-weight: bold
line-height: normal
font-size: 18px
text-align: center

/* positioning */
position: relative
display: inline-block

&.disabled
pointer-events: none
opacity: 0.4

&:hover
box-shadow: 0px 4px 6px $shadow

&:focus
outline: none
@@ -1,7 +1,7 @@
nav
display: flex
background: #FFFFFF
box-shadow: 0px 4px 6px rgba(0, 82, 174, 0.15)
background: $white
box-shadow: 0px 4px 6px $shadow
height: 64px

> div
@@ -2,8 +2,7 @@ import React from 'react';
import { Link } from 'react-router-dom';
import { DISPLAY_TYPE } from '../../containers/navbar/DisplayTypes';
import { BUTTON_TYPE } from '../../containers/navbar/ButtonTypes';
import { SecondaryButton } from '../frontEndComponents/button';
import './Navbar.sass';
import { SecondaryButton } from '../input/buttons';

const getLogo = () => (
<Link to="/"><img alt="" src="../../assets/logo.png" /></Link>
@@ -0,0 +1 @@
// Import all container styles here
@@ -0,0 +1,30 @@
$transition-time: .2s

// This is a hack to allow transitions of linear-gradient backgrounds.
// https://medium.com/@dave_lunny/animating-css-gradients-using-only-css-d2fd7671e759
// Use this by @include'ing it in your class with required arguments.
// Arguments:
// $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

background-size: 100%
background-image: $main
position: relative
z-index: 100
&:before
border-radius: inherit
background-image: $hover
content: ''
display: block
height: 100%
position: absolute
top: 0
left: 0
opacity: 0
width: 100%
z-index: -100
-webkit-transition: $transition
&:hover
&:before
opacity: 1
@@ -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

$grey-dark: #363131
$grey-light: #BDBDBD
$grey-slate: #9195A5

/* classes */
$error: #EB5757 // figma "red"
$primary: #21258A // figma "body text"
$secondary: #00BCBC // figma "link text"
$secondary-highlight: #00D5D5 // figma "link text/hover"

/* gradients */
$gradient-primary: linear-gradient(180deg, #4DE8C2 0%, #19CBCB 100%, #18CDCD 100%)
$gradient-primary-highlight: linear-gradient(180deg, #92F1DA 0%, #43DEDE 100%)
$gradient-primary-subdued: linear-gradient(180deg, #4DE8C2 0%, #18CDCD 100%, #19CBCB 100%);
$gradient-secondary: linear-gradient(180deg, #FFFFFF 0%, #EDFFFC 100%)

/* misc */
$shadow: rgba(0, 82, 174, 0.15)
@@ -1,5 +1,13 @@
/* core */
@import ./colors
@import ./typography
@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


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

html, body, div, span, applet, object, iframe,
h1, h2, h3, h4, h5, h6, p, blockquote, pre,
ProTip! Use n and p to navigate between commits in a pull request.
You can鈥檛 perform that action at this time.