feat(Slider): Add Slider component, storybook and tests by Ron-Lavi · Pull Request #280 · patternfly/patternfly-react

Ron-Lavi · 2018-03-19T20:44:47Z

jQuery is no longer an issue since I have published the npm module 'bootstrap-slider-without-jquery'.

coveralls · 2018-03-19T20:49:39Z

Pull Request Test Coverage Report for Build 991

48 of 70 (68.57%) changed or added relevant lines in 4 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.5%) to 71.068%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/components/Slider/Boundaries.js	15	17	88.24%
src/components/Slider/DropdownMenu.js	9	12	75.0%
src/components/Slider/Slider.js	14	21	66.67%
src/components/Slider/BootstrapSlider.js	10	20	50.0%

Totals
Change from base Build 988:	-0.5%
Covered Lines:	1303
Relevant Lines:	1655

💛 - Coveralls

coveralls · 2018-03-19T20:49:39Z

Pull Request Test Coverage Report for Build 1200

55 of 64 (85.94%) changed or added relevant lines in 4 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.08%) to 75.197%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/components/Slider/Slider.js	19	21	90.48%
src/components/Slider/Boundaries.js	13	15	86.67%
src/components/Slider/BootstrapSlider.js	15	20	75.0%

Totals
Change from base Build 1196:	0.08%
Covered Lines:	1529
Relevant Lines:	1850

💛 - Coveralls

Ron-Lavi · 2018-03-19T20:50:39Z

Hey @serenamarie125 and @jgiardino, I reopened this branch due to some issues with the old one,
please check the updated storybook

serenamarie125

@LaViro this looks great! Thanks for your contribution!

AllenBW · 2018-03-21T13:49:07Z

+import React from 'react';
+import Enzyme, { mount } from 'enzyme';
+import Adapter from 'enzyme-adapter-react-16';
+import toJSON from 'enzyme-to-json';


No longer require tojson, snapshot serializer is built in 😋

AllenBW · 2018-03-21T13:49:37Z

+import { Slider } from './index';
+import BootstrapSlider from './BootstrapSlider';
+
+Enzyme.configure({ adapter: new Adapter() });


Unsure if we need this also, I suspect not...

dmiller9911

I do have some concerns about the jQuery issues. It might be nice to either try to contribute to the lib or find another that does not have the jQuery workaround. From a consumer standpoint, I would not like to have to work around it. Additionally, this will be a breaking change and will require a major version since it will throw an error at import for any consumer without jQuery.

dmiller9911 · 2018-03-23T20:47:34Z

+    }
+  }
+
+  _onSlide = value => {


Judging by other methods in this repo I do not believe we are prefixing "private" methods with a _ @michaelkro or @priley86 might have better insight into that though.

that's correct - we have not been prefixing with _ here. I don't see that as necessary, but i'm open to ideas on this.

@dmiller @priley86 I think it is a good indication for function that will be used by other components rather than private ones.

removed the private prefix

dmiller9911 · 2018-03-23T20:49:15Z

+    this._slider = new Slider(this.sliderDiv, {
+      ...this.props
+    });
+    this._slider.on('slide', value => that._onSlide(value));


this can be written as this._slider.on('slide', this._onSlide) anything written like classMethod = () => {} are essentially be autobound to the instance.

@sharvit can you remind me why we chose to use bindMethods syntax for binding class functions? It has been a long time since that was discussed...

to @dmiller9911 's point, I believe many communities have moved to the classMethod = () => {} syntax for auto binding...

dmiller9911 · 2018-03-23T20:49:55Z

+  }
+
+  componentDidMount() {
+    const that = this;


Arrow functions remove the need for doing this.

@dmiller9911 I am using it because the function is being called in bootstrap-slider's context.
this._slider.on('slide', value => that._onSlide(value));

@dmiller9911,
I moved the onSlide function into componentDidMount

dmiller9911 · 2018-03-23T20:54:18Z

+    this._slider.setAttribute('formatter', nextProps.formatter);
+    // Adjust the tooltip to "sit" ontop of the slider's handle. #LibraryBug
+    // check
+    if (this.props.orientation === 'horizontal') {


Creating constants for orientation similar to alerts and referencing the variable here will make it easier to maintain than using a string here.

dmiller9911 · 2018-03-23T20:59:41Z

+  // Instead of rendering the slider element again and again,
+  // we took advantage of the bootstrap-slider library
+  // and only update the new value or format when new props arrive.
+  componentWillUpdate(nextProps, nextState) {


move this to componentDidUpdate. willUpdate will be deprecated in the next major react release, and the warning will be starting the next minor.

big +1 😸

@dmiller9911 Moved it to componentWillReceiveProps

dmiller9911 · 2018-03-23T21:09:10Z

+    }
+  }
+
+  const slider = children[0];


it is usually bad practice to assume children's data structure in React. React exports tools for this. Wou will want to run children through this: https://reactjs.org/docs/react-api.html#reactchildrentoarray before referencing the array positions. That said for this scenario is might be better to have a slider Prop and a form Prop or slider and then use children for form.

<Bounderies slider={<BSSlider />} >form goes here</Bounderies>

dmiller9911 · 2018-03-23T21:13:00Z

+      );
+    }
+
+    const inputElement = this.props.input ? (


This can be written as !!this.props.input && () React will render false the same as null.

dmiller9911 · 2018-03-23T21:15:17Z

+    dropdownList: ['MB', 'GB'],
+    dropup: true
+  };
+  const wrapper = mount(<Slider {...props} />);


any way to use shallow here and mock out bootstap-slider to return a mock instance/methods?

dmiller9911 · 2018-03-23T21:17:03Z

+    'componentWillUpdate'
+  );
+  innerSlider.setProps({ value: 60 });
+  expect(componentWillUpdate).toHaveBeenCalled();


This doesn't really test much since it is kind of just exercising enzyme/react's lifecycle. A better test would be to verify the changes we make to slider after an update. You would need to get Slider mocked though. If you need help with that let me know. I can probably help out with that.

@dmiller9911, I was also thinking about it, but it seems that Coverall's rate reduced when I didn't test it.

dmiller9911 · 2018-03-23T21:18:37Z

+    dropup: true
+  };
+  const wrapper = mount(<Slider {...props} />);
+  expect(toJSON(wrapper)).toMatchSnapshot();


This snapshot is too big if we can't get shallow working. It would be better to verify values/method calls. It is best to try and keep snapshots around 20 lines or below.

Ron-Lavi · 2018-03-25T08:16:14Z

@dmiller9911 @priley86 @AllenBW Thank you for your review guys!

mturley · 2018-03-26T22:31:53Z

@LaViro this looks great! I do agree with @dmiller9911 however, about the jQuery issue... We definitely don't want consumers of this repo to have to think about jQuery whatsoever. Having to make an exception around jQuery or get a jQuery based warning... Both are no good. It would be nice to find a way to cut jQuery out of the picture entirely-- even if we need to target our own new fork of the slider or something.

Ron-Lavi · 2018-03-27T14:17:27Z

@mturley @dmiller9911 Thanks, I am going to try to figure this out.. Tell me if you got any idea

Ron-Lavi · 2018-04-09T12:57:24Z

@dmiller9911 @mturley I hacked the 'bootstrap-slider' package and published a new package called 'bootstrap-slider-without-jquery' which is jQuery-free.

jeff-phillips-18 · 2018-04-17T12:59:03Z

Need to add export * from './components/Slider'; to src/index.js

Ron-Lavi · 2018-04-17T13:15:59Z

@jeff-phillips-18 It's the most important thing :)

…brary, add storybook and tests. BREAKING CHANGE: Although jQuery is optional to this library, when it is not being used, an error will be raised. In order to fix this error, we need to set jquery to null in webpack. Please check the library repo on github for more information about this issue. fix patternfly#183

-Delete constructor. -Add orientation enums. -Change moved onSlide into componentDidMount and change the syntax of the binded call. -Change componentWillUpdate, as it is deprecated, to componentWillRecieveProps. -Change the private _ convention. -Add formatter props validation. ------------------------------------------------------------------------ Change Boundaries.js: -Add slider prop. -Delete avoid use of children index. ------------------------------------------------------------------------ Change Slider.js: -Add implementation to Boundaries slider prop. -Change moved BSSlider to const. ------------------------------------------------------------------------ Change Slider.test.js: -Delete unnecessary imports and configurations. -Change props to inline props.

- Change snapshot was decreased from 360 lines to 100. - Add test for slide and slideStop

-Change the import source of Dropdown and MenuItem to '../../index'.

Ron-Lavi force-pushed the feature/slider branch 3 times, most recently from 3e2322b to dbf780b Compare March 19, 2018 22:13

serenamarie125 previously approved these changes Mar 20, 2018

View reviewed changes

serenamarie125 added the ux approved label Mar 20, 2018

AllenBW reviewed Mar 21, 2018

View reviewed changes

dmiller9911 suggested changes Mar 23, 2018

View reviewed changes

Ron-Lavi dismissed serenamarie125’s stale review via 1ca0d99 March 27, 2018 15:34

Ron-Lavi force-pushed the feature/slider branch from dbf780b to 1ca0d99 Compare March 27, 2018 15:34

Ron-Lavi force-pushed the feature/slider branch from 1ca0d99 to 72b46db Compare April 9, 2018 12:45

Ron-Lavi changed the title ~~feat(Slider): Add Slider component, storybook and tests~~ [W.I.P] feat(Slider): Add Slider component, storybook and tests Apr 9, 2018

Ron-Lavi changed the title ~~[W.I.P] feat(Slider): Add Slider component, storybook and tests~~ feat(Slider): Add Slider component, storybook and tests Apr 11, 2018

Ron-Lavi self-assigned this Apr 11, 2018

ronlavi2412 added 4 commits April 17, 2018 17:23

- Add mock for the 'bootstrap-slider-without-jquery' module.

c3d7a55

- Change snapshot was decreased from 360 lines to 100. - Add test for slide and slideStop

Add an export of Slider to 'src/index.js'

2a6a3ef

Ron-Lavi force-pushed the feature/slider branch from 81a86d6 to 2a6a3ef Compare April 17, 2018 14:49

jeff-phillips-18 previously approved these changes Apr 17, 2018

View reviewed changes

Ron-Lavi dismissed jeff-phillips-18’s stale review via eb860db April 17, 2018 16:52

-Add test for dropdown's menuitem click.

cfdebfc

-Change the import source of Dropdown and MenuItem to '../../index'.

Ron-Lavi force-pushed the feature/slider branch from eb860db to cfdebfc Compare April 17, 2018 17:08

dmiller9911 approved these changes Apr 17, 2018

View reviewed changes

jeff-phillips-18 approved these changes Apr 17, 2018

View reviewed changes

jeff-phillips-18 merged commit c27866d into patternfly:master Apr 17, 2018

Conversation

Ron-Lavi commented Mar 19, 2018 • edited by jeff-phillips-18 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Mar 19, 2018

Pull Request Test Coverage Report for Build 991

💛 - Coveralls

Uh oh!

coveralls commented Mar 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 1200

💛 - Coveralls

Uh oh!

Ron-Lavi commented Mar 19, 2018

Uh oh!

serenamarie125 left a comment

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!

dmiller9911 left a comment

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!

priley86 Mar 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Ron-Lavi Mar 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ron-Lavi commented Mar 25, 2018

Uh oh!

mturley commented Mar 26, 2018

Uh oh!

Ron-Lavi commented Mar 27, 2018

Uh oh!

Ron-Lavi commented Apr 9, 2018

Uh oh!

jeff-phillips-18 commented Apr 17, 2018

Uh oh!

Ron-Lavi commented Apr 17, 2018

Uh oh!

Reviewers

Ron-Lavi commented Mar 19, 2018 •

edited by jeff-phillips-18

Loading

coveralls commented Mar 19, 2018 •

edited

Loading

priley86 Mar 24, 2018 •

edited

Loading

Ron-Lavi Mar 27, 2018 •

edited

Loading