Stepper component #83
Conversation
| "classnames": "^2.1.0", | ||
| "font-awesome": "^4.3.0", | ||
| "grunt": "^0.4.5", | ||
| "grunt-shell": "^1.1.1", | ||
| "gruntfile-gtx": "^0.3.0", | ||
| "jest-cli": "^0.5.10", |
There was a problem hiding this comment.
Why has Jest been introduced again?
There was a problem hiding this comment.
Not something I remember adding in, I have removed the dependency.
|
Dev |
|
I'm getting the same issue as before where the stepper loses the event handler. This time it only seems to happen when you hit the limiters |
| <CustomComponent codeText={fs.readFileSync(__dirname + '/../examples/Stepper.jsx', 'utf8')} /> | ||
| <h4>Attributes</h4> | ||
| <ul> | ||
| <li><code>value</code> Number - The value of the stepper</li> |
There was a problem hiding this comment.
Should this read "The initial value of the stepper"?
There was a problem hiding this comment.
I think this is ok, the onChange call will re-render the stepper passing in a new actual value...
|
@cameronviner suggested removing |
|
taking a review |
|
Trading have a new requirement for their wizard, a stepper which displays a value with text (eg "2am"). How difficult would it be to extend this to match this new use case? I'm not suggesting we do it here, merely asking your thoughts. |
|
|
||
| getInitialState: function() { | ||
| return { | ||
| value: this.props.value |
There was a problem hiding this comment.
this one is an antipattern, I have been reliably informed! I think we can get away without the stepper maintaining its own state at all. Only use this.props.value and do not increment it but call the onChange with this.props.value + 1 or this.props.value - 1. Let me check this with someone with more react experience though.
There was a problem hiding this comment.
consider that checked! Get rid of state completely here, it simplifies your code. Then one more general point, if you run coverage you'll see if your new test covers all the possibilities - right now it does not.
npm run coveralls;
open coverage/lcov-report/index.html
some of the other tests do not cover enough either, that's not your problem and I will fix that, but you should aim for 100% coverage with your stepper test. When you get rid of state you will find you have less code to cover so your test might even get simpler.
coveralls is the name of the npm script here though it's likely to change to npm run coverage soon if we standardise...
|
to cover displaying an alternative value to the actual number you're incrementing (2am, 5 cakes, etc) you might pass in an optional transform function... easy enough I think, but shouldn't bog down this release imo |
|
@hxpaul @hxtomprice I've updated this now with your changes and better test coverage (an empty function is stopping this getting 100%). Could you take another look when you get a chance? |
| "react-data-attributes-mixin": "git://github.com/holidayextras/react-data-attributes-mixin", | ||
| "reactify": "~1.1.0", | ||
| "redirectify": "^1.4.0", |
There was a problem hiding this comment.
don't think this is needed is it?
|
it is a thing of beauty now 👍 |
|
👍 |
|
Seen it working too, I'm using it in the hotel wizard now... |
What does this PR do? (please provide any background)
Adds a stepper component to to the UI Toolkit
What tests does this PR have?
Unit tests added to cover the new component
How can this be tested?
checkout the branch and then run:
grunt docsscroll to the bottom of the page and test the steppers functionality.
Screenshots / Screencast
What gif best describes how you feel about this work?
Developer Definition of Done/Quality Checklist (for PR author to complete BEFORE code review):
Software Engineer or Developer review:
Software Engineer or project guru review: