-
Notifications
You must be signed in to change notification settings - Fork 27
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
Fix issues with updating template images #338
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.
Testing went well, I couldn't find any BOTH softbuttons overflowing.
This branch needs to be rebased on develop since we squashed the commits from the PR templates.
src/js/Templates/Shared/Image.js
Outdated
@@ -7,13 +7,21 @@ class Image extends React.Component { | |||
constructor(props) { | |||
super(props); | |||
this.state = {error: false, refreshed: false}; | |||
this.canvasStyle = { | |||
this.customCanvasStyle = { |
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.
@JackLivio can you clarify the purpose of this variable? Is the intention that someone who adopts the generic HMI will modify the customCanvasStyle value here in the constructor to hardcode canvas sizes for the whole HMI?
This variable currently has no effect on how an Image is rendered.
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.
@iCollin Referring to canvasStyle before the changes in the pr, that object was created to store the canvasContainer height and width. There was a strange issue in safari where the canvasContainer height would increase when the image in a button that was clicked on, and it would drive the image down the page.
45dd957
to
6f5438e
Compare
@iCollin Rebased commits, unfortunately it did require a force-push, but the changes were directly cherry-picked from the 3 original commits |
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.
Testing went well, approved.
One question I have though, do you think we should leave the default canvas size as 300? I mistyped my image name at first, and the text was forced outside of an alert softbutton because by default the canvas was 300x75. This would also apply in the case an image upload failed. Would it make sense to use a smaller default canvas size?
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 could not reproduce #293 on this branch. I think we can remove customCanvasStyle
.
Fixes #323
Summary of Changes:
Image.js