-
Notifications
You must be signed in to change notification settings - Fork 429
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[imagetool] Make imagetool responsive #590
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of comments, otherwise LGTM. I'm just gonna test this a bit locally before approving.
@@ -39,6 +40,7 @@ export default class HotspotImage extends React.PureComponent { | |||
static defaultProps = { | |||
alignX: 'center', | |||
alignY: 'center', | |||
className: undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to specify undefined as default prop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get a warning from eslint if I don't.
@@ -136,7 +138,7 @@ export default class HotspotImage extends React.PureComponent { | |||
} | |||
}) | |||
return ( | |||
<div className={className} style={style} ref={this.setContainerElement}> | |||
<div className={className || styles.root} style={style} ref={this.setContainerElement}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we merge the classNames? That's the pattern we use elsewhere I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -15,6 +16,41 @@ const MARGIN_PX = 8 | |||
const CROP_HANDLE_SIZE = 12 | |||
const HOTSPOT_HANDLE_SIZE = 10 | |||
|
|||
function checkCropBounderies(value, delta) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo bounderies should be boundaries (or just bounds for short)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo still there?
return true | ||
} | ||
|
||
function limitToBounderies(value, delta) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same typo as mentioned above
* [form-builder] Styling imageToolInput * [imagetool] Make sure the container takes all width * [imgetool] Keep things inside boundries * [imagetool] Touch support * [imagetool] Canvas as block * [imagetool] CSS-file and wrapper. fix position bug * [form-builder] Not so high image tool input * [imagetool] Changes requested in PR * [imagetool] Typo
* [form-builder] Styling imageToolInput * [imagetool] Make sure the container takes all width * [imgetool] Keep things inside boundries * [imagetool] Touch support * [imagetool] Canvas as block * [imagetool] CSS-file and wrapper. fix position bug * [form-builder] Not so high image tool input * [imagetool] Changes requested in PR * [imagetool] Typo
Some bugs are still present: