MM-15709 certain aspect ratio of images cause pop on firefox #2826

sudheerDev · 2019-05-20T22:19:45Z

Dimensions here are numbers and need to have px appended so that the styles are applied

https://mattermost.atlassian.net/browse/MM-15709

* Svg's need max dimention styles with px suffix

jespino · 2019-05-21T06:11:03Z

components/size_aware_image.jsx

@@ -120,7 +120,7 @@ export default class SizeAwareImage extends React.PureComponent {
                    <svg
                        xmlns='http://www.w3.org/2000/svg'
                        viewBox={`0 0 ${dimensions.width} ${dimensions.height}`}
-                        style={{verticalAlign: 'middle', maxHeight: `${dimensions.height}`, maxWidth: `${dimensions.width}`}}
+                        style={{verticalAlign: 'middle', maxHeight: `${dimensions.height}px`, maxWidth: `${dimensions.width}px`}}


It is more straight forward and more "idiomatic" in react to set the length in pixels just as integers, and leave react to add the (px at the end).

Suggested change

style={{verticalAlign: 'middle', maxHeight: `${dimensions.height}px`, maxWidth: `${dimensions.width}px`}}

style={{verticalAlign: 'middle', maxHeight: dimensions.height, maxWidth: dimensions.width}}

i am 0/5 on this but that does not seem "idiomatic" to me. Explicit declaration seemed more natural.

React used to add px to string as well and now apparently they don't do it anymore. Just wondering if they stop deciding to that with numbers we have to all change of these instances. I was checking few issues in react where devs are suggesting to remove this. I feel we should keep it explicit by adding them @jespino

At least it is being considered for adding a warning to explicitly add units in dev env facebook/react#13567

I think the discussion goes more in the way of do not explicitly add "px" for every single number and only do it for certain css properties, like Width, Height, Border, Margin and Padding (and all its variants). I still prefer to use numbers right away, but probably makes sense to hear a third opinion here. @bradjcoughlin what do you think about it?

I'm 1/5 leaning towards specifying px for any property ~~not~~ on this list: https://github.com/facebook/react/blob/4131af3e4bf52f3a003537ec95a1655147c81270/src/renderers/dom/shared/CSSProperty.js#L15-L59
as referenced in the React docs:
https://reactjs.org/docs/dom-elements.html#style

What that says is actually the opposite of that. It says, if you need other unit than px you must specify it, if not, you can use a regular number, and add the exceptions lists to clarify where it doesn't auto include the px.

@jespino right. Not sure why I negated that statement : )

@jespino I feel this would be hard as we have to check for supported properties while programming as well as when reviewing making this harder than needed. Is your suggestion to at-least add it for obvious properties? Final thoughts?

I think extra "thinking" is not needed. The approach is, if your unit is "px", you can use directly a number, if you need to specify the unit, you pass it explicitly. It will add the "px" for that properties where it makes sense, and leave as a number for the properties where it doesn't makes sense.

jespino

Proposed an improvement.

sudheerDev · 2019-05-22T11:15:24Z

@jespino @bradjcoughlin Changed the values to numbers. We kinda need this PR merged ASAP so QA can start testing few blocked tasks. We can continue with the discussion but can we go ahead with the PR if there are no functional changes required?. I can create a follow up PR if needed once we come to an agreement on the idiomatic way of doing this.

* MM-155703 certain aspect ratio of images cause pop on firefox * Svg's need max dimention styles with px suffix * remove string convertion and keep maxHeight and maxWidth as numbers

MM-155703 certain aspect ratio of images cause pop on firefox

8fa16ac

* Svg's need max dimention styles with px suffix

sudheerDev added 2: Dev Review Requires review by a core commiter CherryPick/Candidate A candidate for a quality or patch release, but not yet approved labels May 20, 2019

sudheerDev added this to the v5.12.0 milestone May 20, 2019

sudheerDev requested review from jespino and bradjcoughlin May 20, 2019 22:19

jespino reviewed May 21, 2019

View reviewed changes

jespino suggested changes May 21, 2019

View reviewed changes

sudheerDev changed the title ~~MM-155703 certain aspect ratio of images cause pop on firefox~~ MM-15709 certain aspect ratio of images cause pop on firefox May 21, 2019

sudheerDev force-pushed the MM-15703 branch from 91a5841 to 8fa16ac Compare May 21, 2019 08:07

amyblais added CherryPick/Approved Meant for the quality or patch release tracked in the milestone and removed CherryPick/Candidate A candidate for a quality or patch release, but not yet approved labels May 21, 2019

remove string convertion and keep maxHeight and maxWidth as numbers

f085113

jespino approved these changes May 22, 2019

View reviewed changes

bradjcoughlin approved these changes May 22, 2019

View reviewed changes

sudheerDev added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter labels May 22, 2019

sudheerDev merged commit 0dab1a5 into mattermost:master May 22, 2019

sudheerDev deleted the MM-15703 branch May 22, 2019 15:20

sudheerDev added CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone and removed CherryPick/Approved Meant for the quality or patch release tracked in the milestone labels May 22, 2019

amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels May 22, 2019

ogi-m added the Tests/Done Release tests have been written label May 31, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MM-15709 certain aspect ratio of images cause pop on firefox #2826

MM-15709 certain aspect ratio of images cause pop on firefox #2826

sudheerDev commented May 20, 2019

jespino May 21, 2019

sudheerDev May 21, 2019

sudheerDev May 21, 2019 •

edited

Loading

sudheerDev May 21, 2019

jespino May 21, 2019

bradjcoughlin May 22, 2019 •

edited

Loading

jespino May 22, 2019

bradjcoughlin May 22, 2019

sudheerDev May 22, 2019 •

edited

Loading

jespino May 28, 2019

jespino left a comment

sudheerDev commented May 22, 2019

	style={{verticalAlign: 'middle', maxHeight: `${dimensions.height}px`, maxWidth: `${dimensions.width}px`}}
	style={{verticalAlign: 'middle', maxHeight: dimensions.height, maxWidth: dimensions.width}}

MM-15709 certain aspect ratio of images cause pop on firefox #2826

MM-15709 certain aspect ratio of images cause pop on firefox #2826

Conversation

sudheerDev commented May 20, 2019

jespino May 21, 2019

Choose a reason for hiding this comment

sudheerDev May 21, 2019

Choose a reason for hiding this comment

sudheerDev May 21, 2019 • edited Loading

Choose a reason for hiding this comment

sudheerDev May 21, 2019

Choose a reason for hiding this comment

jespino May 21, 2019

Choose a reason for hiding this comment

bradjcoughlin May 22, 2019 • edited Loading

Choose a reason for hiding this comment

jespino May 22, 2019

Choose a reason for hiding this comment

bradjcoughlin May 22, 2019

Choose a reason for hiding this comment

sudheerDev May 22, 2019 • edited Loading

Choose a reason for hiding this comment

jespino May 28, 2019

Choose a reason for hiding this comment

jespino left a comment

Choose a reason for hiding this comment

sudheerDev commented May 22, 2019

sudheerDev May 21, 2019 •

edited

Loading

bradjcoughlin May 22, 2019 •

edited

Loading

sudheerDev May 22, 2019 •

edited

Loading