Skip to content

Commit

Permalink
Removed usage of responsive mixin
Browse files Browse the repository at this point in the history
  • Loading branch information
@akiran
akiran committed Apr 14, 2017
1 parent cbfc3e7 commit b798b1f
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 27 deletions.
4 changes: 1 addition & 3 deletions package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
"css-loader": "^0.25.0",
"deepmerge": "^1.1.0",
"del": "^2.2.2",
"enquire.js": "^2.1.6",

This comment has been minimized.

Sign in to view

Copy link
@afc163

afc163 Apr 19, 2017 •
edited
Loading

Contributor

Should put in dependencies!

#719

This comment has been minimized.

Sign in to view

Copy link
@ikobe-zz

ikobe-zz Apr 19, 2017

Should put in dependencies...

This comment has been minimized.

Sign in to view

Copy link
@eternalsky

eternalsky Apr 19, 2017

Should put in dependencies!
"enzyme": "^2.4.1",
"es5-shim": "^4.5.9",
"eslint": "^3.6.1",
Expand All @@ -65,11 +66,8 @@
"dependencies": {
"classnames": "^2.2.5",
"create-react-class": "^15.5.2",
"enzyme": "^2.8.1",
"json2mq": "^0.2.0",
"object-assign": "^4.1.0",
"react-responsive-mixin": "^0.4.0",
"react-test-renderer": "^15.5.4",
"slick-carousel": "^1.6.0"
},
"peerDependencies": {
Expand Down
56 changes: 32 additions & 24 deletions src/slider.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,22 +4,26 @@ import React from 'react';
import {InnerSlider} from './inner-slider';
import assign from 'object-assign';
import json2mq from 'json2mq';
import ResponsiveMixin from 'react-responsive-mixin';
import createReactClass from 'create-react-class';
import defaultProps from './default-props';
import enquire from 'enquire.js';

var Slider = createReactClass({
mixins: [ResponsiveMixin],
innerSlider: null,
innerSliderRefHandler: function (ref) {
this.innerSlider = ref;
},
getInitialState: function () {
return {
export default class Slider extends React.Component {

This comment has been minimized.

Sign in to view

Copy link
@devrelm

devrelm Jul 3, 2017

@akiran This should have been a new major version. module.exports = is not the same as export default, and the change from the former to the latter broke my build.

With module.exports =:

let ReactSlick = require('react-slick');

// ReactSlick is now a reference to the Slider class

With export default:

let ReactSlick = require('react-slick');

// ReactSlick is now an object containing a `default` property
// that is a reference to the Slider class.
constructor(props) {
super(props)
this.state = {
breakpoint: null
};
},
componentWillMount: function () {
this._responsiveMediaHandlers = [];
this.innerSliderRefHandler = this.innerSliderRefHandler.bind(this)
}
innerSliderRefHandler(ref) {
this.innerSlider = ref;
}
media(query, handler) {
enquire.register(query, handler);
this._responsiveMediaHandlers.push({query, handler});
}
componentWillMount() {
if (this.props.responsive) {
var breakpoints = this.props.responsive.map(breakpt => breakpt.breakpoint);
breakpoints.sort((x, y) => x - y);
Expand All @@ -33,7 +37,7 @@ var Slider = createReactClass({
}
this.media(bQuery, () => {
this.setState({breakpoint: breakpoint});
});
})
});

// Register media query for full screen. Need to support resize from small to large
Expand All @@ -43,21 +47,27 @@ var Slider = createReactClass({
this.setState({breakpoint: null});
});
}
},
}

slickPrev: function () {
componentWillUnmount() {
this._responsiveMediaHandlers.forEach(function(obj) {
enquire.unregister(obj.query, obj.handler);
});
}

slickPrev() {
this.innerSlider.slickPrev();
},
}

slickNext: function () {
slickNext() {
this.innerSlider.slickNext();
},
}

slickGoTo: function (slide) {
slickGoTo(slide) {
this.innerSlider.slickGoTo(slide)
},
}

render: function () {
render() {
var settings;
var newProps;
if (this.state.breakpoint) {
Expand Down Expand Up @@ -90,6 +100,4 @@ var Slider = createReactClass({
);
}
}
});

module.exports = Slider;
}

7 comments on commit b798b1f

@stevemao
Copy link

Choose a reason for hiding this comment

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

Could you make a new release? The latest release uses the package create-react-class which doesn't set displayName on the component. So it's a regression. See facebook/react#9384 (comment) and babel/babel#5554

@akiran
Copy link
Owner Author

@akiran akiran commented on b798b1f Apr 18, 2017

Choose a reason for hiding this comment

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

Still we are using mixins in https://github.com/akiran/react-slick/blob/master/src/inner-slider.js
We have to refactor it to avoid mixins usage and before removing create-react-class

@stevemao
Copy link

Choose a reason for hiding this comment

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

Meanwhile we can set displayName on the component manually to fix the regression 😄

@danez
Copy link

@danez danez commented on b798b1f Apr 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Yes please release a new version. In the latest version you have dependencies on enzyme and react-test-render which pulls in all the test stuff on production for us and because of that we now have a missing peer dependency because react-test-render has a peer dependency on [email protected], but we use 15.4.
We reverted for now to 0.14.7.

@iblack10
Copy link

Choose a reason for hiding this comment

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

Should enquire.js be a regular dependency rather than a devDependency?

@skyFi
Copy link

@skyFi skyFi commented on b798b1f Apr 20, 2017

Choose a reason for hiding this comment

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

but cause #273 !!

@WangNingning1994
Copy link

Choose a reason for hiding this comment

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

but cause #273 !!

Please sign in to comment.