Fix #1672 BackgroundSwitcher review #1823
Conversation
| * } | ||
| */ | ||
|
|
||
| const BackgroundSelectorPlugin = connect((state) => ({ |
| }; | ||
| }, | ||
| componentWillUnmount() { | ||
| this.props.setControlProperty('backgroundSelector', 'currentLayer', {}); |
There was a problem hiding this comment.
use a separate reducer or bind this method to background selector from the plugin. I don't want to make components aware of the setControlProperty method if it is possible.
|
|
||
| const BackgroundSelector = React.createClass({ | ||
| propTypes: { | ||
| drawerWidth: React.PropTypes.number, |
There was a problem hiding this comment.
Something a component shouldn't know. Style should be enough to control all we need from the external selector
| bottom: React.PropTypes.number, | ||
| isMobile: React.PropTypes.bool, | ||
| enabled: React.PropTypes.bool, | ||
| drawerEnabled: React.PropTypes.bool, |
There was a problem hiding this comment.
Something a component shouldn't know. Use style instead to get the left position
| drawerWidth: React.PropTypes.number, | ||
| start: React.PropTypes.number, | ||
| bottom: React.PropTypes.number, | ||
| isMobile: React.PropTypes.bool, |
There was a problem hiding this comment.
Something a component shouldn't know, can we extract the differences in one or more specific properties?
| currentLayer: React.PropTypes.object, | ||
| tempLayer: React.PropTypes.object, | ||
| size: React.PropTypes.object, | ||
| desktop: React.PropTypes.object, |
There was a problem hiding this comment.
Something a component shouldn't know
| tempLayer: React.PropTypes.object, | ||
| size: React.PropTypes.object, | ||
| desktop: React.PropTypes.object, | ||
| mobile: React.PropTypes.object, |
There was a problem hiding this comment.
Something a component shouldn't know
| const AerialWithLabels = require('./images/mapthumbs/AerialWithLabels.jpg'); | ||
| const OpenTopoMap = require('./images/mapthumbs/OpenTopoMap.jpg'); | ||
|
|
||
| const thumbs = { |
There was a problem hiding this comment.
All of these should not stay here. Maybe in the plugin
|
|
||
| return {pagination, listSize, visibleIconsLength}; | ||
| }, | ||
| renderDesktop() { |
There was a problem hiding this comment.
Let's create one render method where properties can create both desktop and mobile versions. In the plugin, set properties for desktop or mobile.
| }; | ||
| }, | ||
| render() { | ||
| let glyph = this.props.direction ? 'chevron-right' : 'chevron-left'; |
There was a problem hiding this comment.
const glyph = glyphs[this.props.vertical][this.props.direction];
| }, | ||
| render() { | ||
| const containerClass = this.props.vertical ? 'background-preview-icon-container-vertical' : 'background-preview-icon-container-horizontal'; | ||
| let type = this.props.layer.visibility ? containerClass + ' bg-primary' : containerClass + ' bg-body'; |
There was a problem hiding this comment.
Use const, let is not needed. You can do everything without calculating type twice
web/client/plugins/DrawerMenu.jsx
Outdated
| }; | ||
| }, | ||
| componentWillUnmount() { | ||
| this.props.resetWidth(); |
There was a problem hiding this comment.
I don't think this is needed. If the control is not enabled, width is 0, we don't need an additional property
No description provided.