Spinner button #471

sonhanguyen · 2016-12-12T05:35:43Z

Changes
Add SpinnerButton component
Background context
In Alexandria we now have a spinner component that comes in three sizes.
In Adslot UI we have buttons.

We'd now like to export a new SpinnerButton, one that allows us to show progress.

Trello Cards
https://trello.com/c/FoelX6UK/5932-adslot-ui-spinner-button-component

What it currently looks like:

What it is expected to look like after https://github.com/Adslot/alexandria/pull/356 is merged

OndrejCholeva · 2016-12-12T05:48:47Z

src/components/adslotUi/SpinnerButtonComponent.jsx

+import 'styles/adslotUi/SpinnerButton.scss';
+import Button from './ButtonComponent';
+
+const isBtnProp = (val, key) => Button.propTypes[key];


isButtonProp = (value, key) => Button.propTypes[key]

vinteo · 2016-12-12T05:48:51Z

src/components/adslotUi/ButtonComponent.jsx

@@ -0,0 +1 @@
+module.exports = require('react-bootstrap/lib/Button');


just do import Button from 'react-bootstrap/lib/Button';' below and remove this file

pjivers · 2016-12-12T05:50:35Z

src/components/adslotUi/SpinnerButtonComponent.jsx

+    loadingElement,
+    loadingHoverElement = children,
+  //...btnProps
+  } = props;


Destructure props as arguments...

const SpinnerButton = ({ isLoading, children, ...etc... }) =>

Also, set default value of className (className = '') as

SpinnerButton.defaultProps = { className: '', };

Actually, nevermind. I didn't see you were using props elsewhere. Instead, just put props argument in parentheses... SpinnerButton = (props) => {

I couldn't find out how to destructure argument and get the argument at the same time

const SpinnerButton = ({ isLoading, children, ...etc... }) => { // I need the props object here }

pjivers · 2016-12-12T05:55:42Z

src/components/adslotUi/SpinnerButtonComponent.jsx

+  const size = t.oneOf(['lg', 'large'])(props.bsSize)? null: 'medium'
+  loadingElement = React.cloneElement(loadingElement, { size })
+  */
+  const btnProps = _.pick(props, isBtnProp);


buttonProps

pjivers · 2016-12-12T05:56:33Z

src/components/adslotUi/SpinnerButtonComponent.jsx

+      {...btnProps}
+      className={`spinner-button ${className}`}
+    >
+      <div className="spinner-button__wrapper" style={{ visibility: isLoading ? 'initial' : 'hidden' }}>


Change class name to spinner-button-wrapper. And the next two.

Also, don't use style attribute. Add a class that changes the style.

pjivers · 2016-12-12T05:58:12Z

src/components/adslotUi/SpinnerButtonComponent.jsx

+        <div className="spinner-button__loading">{loadingElement}</div>
+        <div className="spinner-button__reveal">{loadingHoverElement}</div>
+      </div>
+      <div style={{ visibility: isLoading ? 'hidden' : 'initial' }}>{children}</div>


Same here... use classes to change style.

OndrejCholeva · 2016-12-12T06:03:34Z

src/components/adslotUi/SpinnerButtonComponent.jsx

+        <div className="spinner-button__loading">{loadingElement}</div>
+        <div className="spinner-button__reveal">{loadingHoverElement}</div>
+      </div>
+      <div style={{ visibility: isLoading ? 'hidden' : 'initial' }}>{children}</div>


can we just not render elements rather than hiding them?

isLoading ? <div className="spinner-button__wrapper"> <div className="spinner-button__loading">{loadingElement}</div> <div className="spinner-button__reveal">{loadingHoverElement}</div> </div> : <div>{children}</div>

(pseudocode)

We need to preserve the original size of the element. If we hide/remove the text part of the button the size will shrink. The only alternatives are to measure the size and set it in style.

umm, so let's suppose you're making a button that says [ O Exporting... ] when 'loading' and [ Export ] initially. Wouldn't it render like [ ~~O Exporting...~~ Export ] so effectively as [ _____________ Export ] ?

omgaz · 2016-12-12T05:48:35Z

src/examples/exampleEntry.js

  ExampleForm,
  ExampleSelect,
+  ExampleSpinnerButton,


No need to do this, you can just import the spinner button directly to Main.jsx. These examples were put here to isolate larger, more complicated examples.

I need a stateful component to hold isLoading state but our linter does not allow having two components in Main.jsx. Also the file already has a lot of things.

nm, see what you're doing further down

omgaz · 2016-12-12T05:49:10Z

src/styles/adslotUi/SpinnerButton.scss

@@ -0,0 +1,42 @@
+.spinner-button {
+  &__layer {


These double underscores are not a naming convention we follow.

Also, layer, wrapper(maybe) and reveal aren't particularly helpful names. What are they trying to explain.

omgaz · 2016-12-12T05:49:51Z

test/components/adslotUi/SpinnerButtonComponentTest.jsx

+    expect(onClick.calledOnce).to.equal(true);
+    onClick = sinon.spy();
+    element = shallow(
+      <SpinnerButton isLoading onCLick={onClick}>Test</SpinnerButton>


onClick rather than onCLick

omgaz · 2016-12-12T05:51:02Z

src/styles/adslotUi/SpinnerButton.scss

+      }
+    }
+    // workaround for vertical centering
+    & .spinner-component {


I don't think the & is needed here, the nesting should do this for us

omgaz · 2016-12-12T05:54:49Z

src/components/adslotUi/SpinnerButtonComponent.jsx

+
+  return (
+    <Button
+      disabled={isLoading}


isLoading || btnProps.disabled. This still needs to be disabled if it's passed as a prop to disable

I think the next line covered that

{...btnProps}

omgaz · 2016-12-12T05:55:26Z

src/components/adslotUi/SpinnerButtonComponent.jsx

+    loadingElement,
+    loadingHoverElement = children,
+  //...btnProps
+  } = props;


We generally do destructuring in the arguments of the fn

nm, i've seen yours and patricks comments

omgaz · 2016-12-12T05:57:00Z

src/components/adslotUi/SpinnerButtonComponent.jsx

+import { Spinner } from 'alexandria-adslot';
+import _ from 'lodash';
+import 'styles/adslotUi/SpinnerButton.scss';
+import Button from './ButtonComponent';


Require direct from bootstrap rather than having the one line export file.

I thought having SpinnerButton dependent on AdslotUI's Button rather than Bootstrap's, that way if we diverge from Bootstrap we can simply change that one file and have consistency.

I think one day we will have wrappers for 3rd party components which would make it a single dependency (and provide way to inject/overwrite stuff globally), but we should do it consistently (and purposely).

omgaz · 2016-12-12T06:02:29Z

src/components/adslotUi/SpinnerButtonComponent.jsx

+
+SpinnerButton.defaultProps = {
+  isLoading: false,
+  loadingElement: <Spinner size="small" />,


I don't think we need to pass this as a prop; if we want this to do anything else it'll either be a different component or we update it then.

pjivers · 2016-12-12T06:07:44Z

src/examples/components/spinnerButton.jsx

+        this.setState({ isLoading: true }, () =>
+          setTimeout(() => this.setState({ isLoading: false }), 5000)
+        )
+      ,


Maybe change style to...

onClick: () => { this.setState({ isLoading: true }, () => setTimeout(() => this.setState({ isLoading: false }), 5000) ); },

Just for consistency's sake.
@omgaz Thoughts on this?

pjivers · 2016-12-12T06:09:12Z

src/styles/adslotUi/SpinnerButton.scss

@@ -0,0 +1,42 @@
+.spinner-button {
+  &__layer {


This is not our convention. Please change to &-layer. The same for the remaining styles.

omgaz · 2016-12-12T06:11:34Z

src/components/distributionEntry.js

+import 'styles/_icheck-custom.scss';
+import 'styles/_react-select-custom.scss';
+import 'styles/_react-toggle-custom.scss';
+import 'styles/_react-datepicker-custom.scss';


I believe, SCSS files still need to be required so that webpack creates a separate css file.

I tried dist in both cases and saw only one css ouput, had content of those files. Am I missing something?

Awesome! Just trash your dist file and rebuild; double-check that it's still all good :)

pjivers · 2016-12-12T06:22:24Z

src/examples/components/spinnerButton.jsx

+import React from 'react';
+import { SpinnerButton } from 'components/distributionEntry';
+
+export default class extends React.Component {


Export at the end of the file, not here.

pjivers · 2016-12-12T06:26:31Z

src/styles/adslotUi/SpinnerButton.scss

+      }
+    }
+  }
+}


New line at end of file. 😝

pjivers · 2016-12-12T06:27:48Z

test/components/adslotUi/SpinnerButtonComponentTest.jsx

+  it('should render with defaults', () => {
+    const element = shallow(<SpinnerButton>Test</SpinnerButton>);
+    const hoverElement = element.find('.spinner-button__reveal');
+    expect(hoverElement.text()).to.equal('Test');


Not that it makes a functional difference, but you can write it as .to.be('Test');

save keystrokes too

just a note that it is not a function, there is to.be.a() but not to.be()

pjivers · 2016-12-12T06:29:58Z

test/components/adslotUi/SpinnerButtonComponentTest.jsx

+    );
+
+    const btnElement = element.find('Button');
+    expect(btnElement.prop('bsStyle')).is.equal('primary');


Try to write the expect in a way that reads like an English sentence:
e.g. "Expect the button element prop style to be 'primary'" reads better than "Expect the button element prop style is equal 'primary'"

So change it to expect(btnElement.prop('bsStyle')).to.be('primary');

The same goes for the others.

pjivers · 2016-12-12T06:32:12Z

test/components/adslotUi/SpinnerButtonComponentTest.jsx

+    const element = shallow(
+      <SpinnerButton loadingNode="loading..">Test</SpinnerButton>
+    );
+    expect(element.children().first().text()).to.contain('loading..');


For future reference, it's better to use the ellipsis character (…)

OndrejCholeva · 2016-12-12T07:39:51Z

src/components/adslotUi/SpinnerButtonComponent.jsx

+  const size = t.oneOf(['lg', 'large'])(props.bsSize)? null: 'medium'
+  loadingElement = React.cloneElement(loadingElement, { size })
+  */
+  const buttonProps = _.pick(props, isButtonProp);


_.pick takes array as props filter, so you can use following instead extra function:
const buttonProps = _.pick(props, _.keys(Button.propTypes));

codecov-io · 2016-12-13T02:26:24Z

Current coverage is 100% (diff: 100%)

No coverage report found for master at 6dceee0.

Powered by Codecov. Last update 6dceee0...61a9705

OndrejCholeva · 2016-12-13T04:02:54Z

src/styles/adslotUi/SpinnerButton.scss

+          }
+
+          &-medium {
+            margin-top: -3px;


should be -5px, see (left -3, right -5):

that spinner is 30x30
the -3px is for 24x24 spinner which is in http://github.comAdslot/alexandria#356

I thought 30 was a bit too big as it leaves no padding for the button.
@omgaz what do you think?

I see what you mean. It's ok for now, maybe we can re-visit it later.

omgaz · 2016-12-13T23:28:44Z

src/components/adslotUi/SpinnerButtonComponent.jsx

+
+  return (
+    <Button
+      disabled={isLoading}


We still want the button to be disabled if it's disabled, not just loading.

props.disabled || props.isLoading

the line right bellow covers that

{...propsIncludingDisabledPassed}

Main.jsx has that disabled example too.

@sonhanguyen yep, although I could not find the line you mention. @omgaz do we will need to pass it in like

disabled={disabled || isLoading}

in which case, you would still need to pull it out of props just above.

pjivers · 2016-12-13T23:36:24Z

Same here... Amend commit instead of pushing new commits. 😉

omgaz · 2016-12-13T23:36:54Z

src/styles/adslotUi/SpinnerButton.scss

@@ -0,0 +1,48 @@
+.spinner-button {
+  @mixin layer-fillparent {


fillparent should be fill-parent. Also, I'm a bit confused about layer fill parent and what it means. I think just fill-area or fill-element or something may do the job just as well.

by layer I mean position absolute, changed to "fill-area" anyway

omgaz · 2016-12-13T23:39:46Z

aside from the layer-fillparent; LGTM

lteacher

LGTM

mdotwills · 2016-12-13T23:49:57Z

test/components/adslotUi/SpinnerButtonComponentTest.jsx

+    expect(element.children().first().text()).to.contains('loading…');
+  });
+
+  it('should be disabled in loading mode', () => {


@omgaz with the design, is there a case where the button is disabled without showing the spinner (disabled prop) prop and disabled showing the spinner (isLoading prop)?

@sonhanguyen depending on Garry's answer:
should be disabled in loading mode when passed isLoading for this test and maybe another test should be disabled when passed isLoading for the disabled prop talked about above.

OndrejCholeva · 2016-12-14T02:50:20Z

src/components/adslotUi/SpinnerButtonComponent.jsx

+    isLoading,
+    children,
+    loadingHoverElement = children,
+  } = props;


oh, I am sorry to come with late request, but can you add support for passing data-test-selector property down to the button? (https://github.com/Adslot/adslot-ui/blob/8fc657aa6cefd760fecff5f5e063d6f0d1ecaa55/src/components/adslotUi/SplitPaneComponent.jsx as inspiration)

loadingHoverElement = children, dts #yeah, I know it is a shortcut, but that's what we use... } = props; ... <Button {..._.pick(props, _.keys(Button.propTypes))} expandDts(dts) ... dts: PropTypes.string

pseudocode

OndrejCholeva · 2016-12-14T04:43:59Z

test/components/adslotUi/SpinnerButtonComponentTest.jsx

+
+  it('should pass props to button', () => {
+    const element = shallow(
+      <SpinnerButton bsStyle="primary" bsSize="lg">Test</SpinnerButton>


can we add the dts prop as well? expecting data-test-selector to be a buttonElement prop?

All react components accept data as do bootstrap components. We should be able to use data-test-selector directly on the component. @sonhanguyen do you just want to verify that data-test-selector gets passed through correctly and without duplication/errors.

omgaz · 2016-12-18T21:55:35Z

Alexandria pr has merged and released as 4.0.1; you can update your npm package in this PR to pull in those changes :)

Also, squash your commits down to one and we should be good to merge

OndrejCholeva reviewed Dec 12, 2016

View reviewed changes

vinteo reviewed Dec 12, 2016

View reviewed changes

pjivers reviewed Dec 12, 2016

View reviewed changes

OndrejCholeva reviewed Dec 12, 2016

View reviewed changes

omgaz reviewed Dec 12, 2016

View reviewed changes

pjivers reviewed Dec 12, 2016

View reviewed changes

omgaz reviewed Dec 12, 2016

View reviewed changes

pjivers reviewed Dec 12, 2016

View reviewed changes

src/styles/adslotUi/SpinnerButton.scss

}

}

}

}

Copy link

Contributor

pjivers Dec 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New line at end of file. 😝

pjivers reviewed Dec 12, 2016

View reviewed changes

OndrejCholeva reviewed Dec 12, 2016

View reviewed changes

OndrejCholeva reviewed Dec 13, 2016

View reviewed changes

sonhanguyen force-pushed the spinner-button branch from 4c545bb to 330482a Compare December 13, 2016 05:17

omgaz reviewed Dec 13, 2016

View reviewed changes

lteacher approved these changes Dec 13, 2016

View reviewed changes

mdotwills reviewed Dec 13, 2016

View reviewed changes

sonhanguyen force-pushed the spinner-button branch 2 times, most recently from 0507793 to f16ce44 Compare December 14, 2016 00:58

OndrejCholeva reviewed Dec 14, 2016

View reviewed changes

sonhanguyen force-pushed the spinner-button branch from 209e8a4 to e1fb7cd Compare December 14, 2016 03:37

OndrejCholeva reviewed Dec 14, 2016

View reviewed changes

sonhanguyen force-pushed the spinner-button branch from e1fb7cd to f936b36 Compare December 15, 2016 01:03

sonhanguyen force-pushed the spinner-button branch from f936b36 to fe3f253 Compare December 20, 2016 03:51

omgaz force-pushed the spinner-button branch 2 times, most recently from a53bac3 to 2be5a1f Compare December 21, 2016 06:36

New: SpinnerButtonComponent

61a9705

omgaz force-pushed the spinner-button branch from 2be5a1f to 61a9705 Compare December 21, 2016 22:18

omgaz merged commit a3a1005 into master Dec 21, 2016

omgaz deleted the spinner-button branch December 21, 2016 22:24

		@@ -0,0 +1 @@
		module.exports = require('react-bootstrap/lib/Button');

Spinner button #471

Spinner button #471

Conversation

sonhanguyen commented Dec 12, 2016

OndrejCholeva Dec 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjivers Dec 12, 2016 • 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

pjivers Dec 12, 2016 • 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

OndrejCholeva Dec 12, 2016 • 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

Choose a reason for hiding this comment

omgaz Dec 12, 2016 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjivers Dec 12, 2016 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjivers Dec 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OndrejCholeva Dec 12, 2016 • edited Loading

Choose a reason for hiding this comment

codecov-io commented Dec 13, 2016 • edited Loading

Current coverage is 100% (diff: 100%)

Choose a reason for hiding this comment

sonhanguyen Dec 13, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonhanguyen Dec 13, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjivers commented Dec 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omgaz commented Dec 13, 2016

lteacher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OndrejCholeva Dec 14, 2016 • edited Loading

Choose a reason for hiding this comment

OndrejCholeva Dec 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omgaz commented Dec 18, 2016

OndrejCholeva Dec 12, 2016 •

edited

Loading

pjivers Dec 12, 2016 •

edited

Loading

pjivers Dec 12, 2016 •

edited

Loading

OndrejCholeva Dec 12, 2016 •

edited

Loading

omgaz Dec 12, 2016 •

edited

Loading

pjivers Dec 12, 2016 •

edited

Loading

pjivers Dec 12, 2016 •

edited

Loading

OndrejCholeva Dec 12, 2016 •

edited

Loading

codecov-io commented Dec 13, 2016 •

edited

Loading

sonhanguyen Dec 13, 2016 •

edited

Loading

sonhanguyen Dec 13, 2016 •

edited

Loading

OndrejCholeva Dec 14, 2016 •

edited

Loading

OndrejCholeva Dec 14, 2016 •

edited

Loading