New Component: Stepper (Spin Button) by Boris-Em · Pull Request #1759 · microsoft/fluentui

Boris-Em · 2017-05-11T02:14:11Z

Pull request checklist

Includes tests
New feature, bugfix, or enhancement
Documentation update

Description of changes

This PR adds a new component: the SpinButton.

Here is an example of a SpinButton:

…ia enter

…ative theming.

…-react into feature/stepper # Conflicts: # apps/fabric-examples/src/AppDefinition.tsx # packages/office-ui-fabric-react/src/components/Slider/Slider.tsx

…ntation or not.

…me more example spinButtons

…ing via keyboard

…e alternative theming." This reverts commit 4f830cd.

Implement Document Title Bar

jair-cazarin · 2017-05-24T15:12:22Z

packages/office-ui-fabric-react/src/components/SpinButton/SpinButton.Props.ts

+}
+
+export interface ISpinButton {
+  value?: number;


nit: jsdoc comments?

jair-cazarin · 2017-05-24T15:13:25Z

packages/office-ui-fabric-react/src/components/SpinButton/SpinButton.test.tsx

+
+describe('SpinButton', () => {
+  function renderIntoDocument(element: React.ReactElement<any>): HTMLElement {
+    const component = ReactTestUtils.renderIntoDocument(element);


why not use shallow and full enzyme rendering?

@jair-cazarin, we followed what other unit tests are doing. Is there a benefit to using Shallow and Enzyme rendering?

yeah, I understand that, perhaps we should just do a separate work to migrate all UTs to enzyme.

jair-cazarin · 2017-05-24T15:14:41Z

packages/office-ui-fabric-react/src/components/SpinButton/SpinButton.test.tsx

+
+    let newCurrentValue = inputDOM.value;
+    expect(currentValue).to.equal(newCurrentValue);
+  });


missing test that gets the value from the component instance when uncontrolled

We added more tests for that.

jair-cazarin · 2017-05-24T15:16:00Z

packages/office-ui-fabric-react/src/components/SpinButton/SpinButton.tsx

+  private _stepDelay = 100;
+  private _formattedValidUnitOptions: string[] = [];
+
+  constructor(props?: ISpinButtonProps) {


why is the argument props optional?

You're right, it shouldn't be optional. Thanks!

jair-cazarin · 2017-05-24T15:19:51Z

packages/office-ui-fabric-react/src/components/SpinButton/SpinButton.tsx

+              iconProps={ incrementButtonIcon }
+              title='Increase'
+              aria-hidden='true'
+              onMouseDown={ () => { this._updateValue(true /* shouldSpin */, this._onIncrement); } }


this is pretty much allocating a new function for every render...does it make sense to make it a class member?

What are the benefits of using a class member?
It would require us to either create two almost similar functions or check the event to see if increase or decrease is triggered, which seems fragile.

As is now, every time it renders is creating a new function that is bind to onMouseDown. If you use a a class method or store it in a field it would not. This typically breaks pure components since you pass a new reference hence shallow reference checks fail.

Gotcha, thanks for the explanation!

jair-cazarin · 2017-05-24T15:20:34Z

packages/office-ui-fabric-react/src/components/SpinButton/SpinButton.tsx

+              className={ css('ms-UpButton', styles.UpButton, (keyboardSpinDirection === KeyboardSpinDirection.up ? styles.active : '')) }
+              disabled={ disabled }
+              iconProps={ incrementButtonIcon }
+              title='Increase'


shouldn't title be localizable/configurable?

Good point. We actually moved the title to the outter component.

jair-cazarin · 2017-05-24T15:22:22Z

packages/office-ui-fabric-react/src/components/SpinButton/SpinButton.tsx

+   * Set the label direction with the gap on the correct side
+   */
+  @autobind
+  private _labelDirectionHelper(): any {


I added a comment before about considering renaming this function to be getLabelDirectionStyle or something like that

jair-cazarin · 2017-05-24T15:23:44Z

packages/office-ui-fabric-react/src/components/SpinButton/SpinButton.tsx

+   */
+  @autobind
+  private _stop() {
+    if (this._currentStepFunctionHandle !== null) {


_currentStepFunctionHandle is a number, should this be explicitly comparing to 0 instead?

Sure, we used 0 instead of null.

jair-cazarin · 2017-05-24T15:25:13Z

packages/office-ui-fabric-react/src/components/SpinButton/SpinButton.tsx

+  @autobind
+  private _handleKeyUp(event: React.KeyboardEvent<HTMLElement>) {
+
+    if (this.props.disabled) {


I added a comment before about collapsing these two if cases.

Sorry about the missing comments, sometimes GitHub collapses comments for no apparent reason, making it hard to see what has been addressed or not.

Boris-Em · 2017-05-25T20:33:53Z

@cschleiden, we've taken a look at the time picker in VSTS mobile, and it feels different than what we're building here. I added a screenshot illustrating the SpinButton to this PR.

jair-cazarin · 2017-05-27T11:16:01Z

packages/office-ui-fabric-react/src/components/SpinButton/SpinButton.Props.ts

-  value?: number;
+  /**
+   * The value of the SpinButton. Use this if you intend to pass in a new value as a result of onChange events.
+   * This value is mutually exclusive to defaultValue. Use one or the other.


I don't know if this description matches the intention of this, can you set the value from here? I thought this was for readonly when using the component as uncontrolled component.

jair-cazarin · 2017-05-27T11:17:03Z

packages/office-ui-fabric-react/src/components/SpinButton/SpinButton.test.tsx

+    ReactTestUtils.Simulate.mouseDown(downButtonDOM);
+    ReactTestUtils.Simulate.mouseUp(downButtonDOM);
+
+    expect(inputDOM.value).to.equal(String(exampleNewValue));


should the value be checked from the component instance rather than the HTML element?

…c-react

…-react into feature/stepper

msftclas · 2017-05-31T07:25:01Z

@Boris-Em,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

dzearing · 2017-06-01T05:31:49Z

I think this conflicted with the fabric-core merge; @Boris-Em can you repair the sass merge? If you are confused @mikewheaton is the expert on the fabric-core sass renames and can help clarify

dzearing · 2017-06-07T15:40:34Z

oh nice finally passing! I will take a look

* First step at stepper implementation. * Add first implementation of stepper. * Add functionality to stepper * Refine the Stepper class and add tests * let's make sure to put focus back on the text field when submitting via enter * Added documentation to Stepper. * Add flexibility to current stepper implementation. * Modified example implementations. * Add aria-valuemax. * Change Stepper to SpinButton. * Add example with unit. * Implement color scheme in the ContextualMenu control to enable alternative theming. * Improvements to SpinButton. * Fix increment function calls. * Add new width optional parameter. * Add label direction. * Fix border. * Add Position enum. * `defaultValue` is now the deciding prop for using the default implementation or not. * onBlur is now onValidate. * Fix tests. * Fix warnings. * Add implementation for labelGap. * Put some polish on the styling, added some icon support, and added some more example spinButtons * Implement the bar and unit tests and component page * Add the ability for the spinButton buttons to look pressed when spinning via keyboard * Revert "Implement color scheme in the ContextualMenu control to enable alternative theming." This reverts commit 4f830cd. * Don't render an empty icon for an icon-less header menu item. * Revert "Implement Document Title Bar" * update some CSS for high contrast in ff and use css utility instead of concatenating string classnames * Fix quotation issue * Fix Spin Button properties table. * Fix Spin Button example code * Use iconProps instead of string * Extracted `spinning` out of state * Add autobind instead of manually binding private functions to this * Change `+` syntax for more explicit `Number()` * Remove unnecessary cast * Fix typos * `incrementButtonIcon` and `decrementButtonIcon` are now IIconProps * Add KeyboardSpinDirection enum * Fix test description * Fix SpinButton tests * Remove unused onChange callback from SpinButton * Revert onChange * Remove old Stepper.ts file * Use module css instead of global * Fix missing word in comment * Callback functions now allow for void return (state to be updated outside) * Use `_async` instead of window * Fix minor rendering issue with browser zoom * Rename `_spinning` to `_spinningByMouse` for clarity * Fix tests * Fix extra space before label * Remove width outside of SpinButton component and fix styling * Add more tests to SpinButton * Fix SpinButton documentation * Fix typo * Fix AppDefinition for SpinButton and Spinner * Add missing documentation to SpinButton title prop * Various SpinButton fixes * Fix SpinButton path for properties * Fix SpinButton styling issues * Remove labelGap property from SpinButton

* Add ComboBox functionality * Make a fix for IE where non-allowFreeform is showing the keypresses... * Update the PR with code review feedback. Simplified the code a lot, decoupled the shared props/memebers from comboBox and Dropdown, and extended BaseAutoFill which allowed DynamicAutoFill to be removed * Fixing some build warnings * Update my example page to explicitly not use two mutually exclusive props * The npm-shrinkwrap.json file automatically updated... commiting * Had a bad copy paste, fixing up the incorrectly logic * Removing the fontFamily aspect of the comboBox and utilizing the onRenderOption instead. The fontFamily aspect was too specific for the generic comboBox * Update based on feedback * Jspurlin jspurlin/combo box (#2) * Enable no implicit any in the utilities package (#1970) * Fix no implicit anys in utilities package. * Rush change * Reverse all shrinkwrap changes except the typings one. * Fix bundle minification to exclude debug warnings. (#1973) * Updating shrinkwrap, rush, and making minify build have production flag to remove debug code. * Updates. * removing lockfile. * dropping to 7. Moving back to npm run build. * Downgrading rush. * Applying package updates. * Website: Update dev.office.com header (#1966) * Use Fabric Core 7.1.0 for the website * Adjust position of caret's in header so that they spin around central axis * Update to latest icon font from dev.office.com for header and move outside out :global{} to fix build issue * Remove a u- prefix that was missed earlier * Update dev.office.com header with the latest navigation links * Add change file * Make sure the quote rule is enabled for tsline (#1961) * With responsive mode error (#1956) * withResponsiveMode: Adding error handling around the case where window.innerWidth throws an exception * adding change log file * Create withResponsiveMode.tsx * New Component: Stepper (#1759) * First step at stepper implementation. * Add first implementation of stepper. * Add functionality to stepper * Refine the Stepper class and add tests * let's make sure to put focus back on the text field when submitting via enter * Added documentation to Stepper. * Add flexibility to current stepper implementation. * Modified example implementations. * Add aria-valuemax. * Change Stepper to SpinButton. * Add example with unit. * Implement color scheme in the ContextualMenu control to enable alternative theming. * Improvements to SpinButton. * Fix increment function calls. * Add new width optional parameter. * Add label direction. * Fix border. * Add Position enum. * `defaultValue` is now the deciding prop for using the default implementation or not. * onBlur is now onValidate. * Fix tests. * Fix warnings. * Add implementation for labelGap. * Put some polish on the styling, added some icon support, and added some more example spinButtons * Implement the bar and unit tests and component page * Add the ability for the spinButton buttons to look pressed when spinning via keyboard * Revert "Implement color scheme in the ContextualMenu control to enable alternative theming." This reverts commit 4f830cd. * Don't render an empty icon for an icon-less header menu item. * Revert "Implement Document Title Bar" * update some CSS for high contrast in ff and use css utility instead of concatenating string classnames * Fix quotation issue * Fix Spin Button properties table. * Fix Spin Button example code * Use iconProps instead of string * Extracted `spinning` out of state * Add autobind instead of manually binding private functions to this * Change `+` syntax for more explicit `Number()` * Remove unnecessary cast * Fix typos * `incrementButtonIcon` and `decrementButtonIcon` are now IIconProps * Add KeyboardSpinDirection enum * Fix test description * Fix SpinButton tests * Remove unused onChange callback from SpinButton * Revert onChange * Remove old Stepper.ts file * Use module css instead of global * Fix missing word in comment * Callback functions now allow for void return (state to be updated outside) * Use `_async` instead of window * Fix minor rendering issue with browser zoom * Rename `_spinning` to `_spinningByMouse` for clarity * Fix tests * Fix extra space before label * Remove width outside of SpinButton component and fix styling * Add more tests to SpinButton * Fix SpinButton documentation * Fix typo * Fix AppDefinition for SpinButton and Spinner * Add missing documentation to SpinButton title prop * Various SpinButton fixes * Fix SpinButton path for properties * Fix SpinButton styling issues * Remove labelGap property from SpinButton * merge some changes * merge changes * merge * Fix a new tslint warning after npm installing * Fixing some casing warnings npm start was angry about * Removing an extra line that got added with the last push * Create SelectableOption.ts * Create SelectableOption.ts * Create ComboBox.Props.ts * Create ComboBox.test.tsx * Create ComboBox.Basic.Example.tsx * Create Dropdown.Props.ts * Fix the case sensitivity issue * one more casing issue * changing the reference of utilities in the test file * Actually it look like it has to be pascalCase here * ... really... what's going on with the casing here * Address feedback from in person review with David * A few minor updated to remove uneeded comment and unneeded try/finally in tests * Fix up a typo and fix up to use consistent syntax on a few lines

Boris Emorine and others added 30 commits March 29, 2017 17:11

First step at stepper implementation.

8e1b55a

Add first implementation of stepper.

d54e9f4

Add functionality to stepper

4369f67

Refine the Stepper class and add tests

41ac16a

let's make sure to put focus back on the text field when submitting v…

0d1412f

…ia enter

Added documentation to Stepper.

8570e56

Add flexibility to current stepper implementation.

f78f686

Modified example implementations.

8e28b99

Add aria-valuemax.

4ccaf1d

Change Stepper to SpinButton.

4e2f418

Add example with unit.

d699fc0

Implement color scheme in the ContextualMenu control to enable altern…

4f830cd

…ative theming.

Improvements to SpinButton.

ffa948d

Fix increment function calls.

40c6f64

Add new width optional parameter.

f002ceb

Add label direction.

486fc93

Fix border.

68e4c1f

Merge branch 'master' of https://github.com/Boris-Em/office-ui-fabric…

e6fbc48

…-react into feature/stepper # Conflicts: # apps/fabric-examples/src/AppDefinition.tsx # packages/office-ui-fabric-react/src/components/Slider/Slider.tsx

Add Position enum.

25cf2cf

defaultValue is now the deciding prop for using the default impleme…

0fe58fe

…ntation or not.

onBlur is now onValidate.

9241663

Fix tests.

21453e3

Fix warnings.

629d21d

Add implementation for labelGap.

8475fc4

Put some polish on the styling, added some icon support, and added so…

94d2adc

…me more example spinButtons

Implement the bar and unit tests and component page

fdaadbd

Add the ability for the spinButton buttons to look pressed when spinn…

6b9716d

…ing via keyboard

Revert "Implement color scheme in the ContextualMenu control to enabl…

238ad26

…e alternative theming." This reverts commit 4f830cd.

Don't render an empty icon for an icon-less header menu item.

0436c11

Merge pull request #1 from Boris-Em/feature/documentTitleThingy

590d005

Implement Document Title Bar

jair-cazarin reviewed May 24, 2017

View reviewed changes

Boris Emorine added 6 commits May 25, 2017 14:13

Add more tests to SpinButton

fbccd1b

Fix SpinButton documentation

0f3561f

Fix typo

faa7700

Fix AppDefinition for SpinButton and Spinner

f359b37

Add missing documentation to SpinButton title prop

a82c296

Various SpinButton fixes

1c56b5e

jair-cazarin approved these changes May 27, 2017

View reviewed changes

Boris Emorine added 3 commits May 30, 2017 11:57

Fix SpinButton path for properties

70edd03

Merge branch 'master' of https://github.com/OfficeDev/office-ui-fabri…

8d5f70c

…c-react

Merge branch 'master' of https://github.com/Boris-Em/office-ui-fabric…

717f233

…-react into feature/stepper

dzearing closed this May 31, 2017

dzearing reopened this May 31, 2017

msftclas added the cla-not-required label May 31, 2017

mikewheaton changed the title ~~Feature/stepper~~ New Component: Stepper Jun 2, 2017

Boris Emorine and others added 2 commits June 6, 2017 16:08

Fix SpinButton styling issues

297d56d

Merge branch 'master' into feature/stepper

102a1b7

micahgodbolt and others added 4 commits June 7, 2017 09:26

Merge branch 'master' into feature/stepper

482e785

Merge branch 'master' into feature/stepper

58562b3

Remove labelGap property from SpinButton

0604770

Merge branch 'master' into feature/stepper

f4d96b4

christiango merged commit 616ab8c into microsoft:master Jun 8, 2017

micahgodbolt changed the title ~~New Component: Stepper~~ New Component: Stepper (Spin Button) Jun 9, 2017

microsoft locked as resolved and limited conversation to collaborators Aug 31, 2019

Conversation

Boris-Em commented May 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull request checklist

Description of changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Boris-Em commented May 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msftclas commented May 31, 2017

Uh oh!

dzearing commented Jun 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dzearing commented Jun 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

Boris-Em commented May 11, 2017 •

edited

Loading

dzearing commented Jun 1, 2017 •

edited

Loading