-
-
Notifications
You must be signed in to change notification settings - Fork 831
Conversation
'use strict'; | ||
|
||
module.exports = { | ||
thumbHeight: function(fullWidth, fullHeight, thumbWidth, thumbHeight) { |
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.
Now that this is a public function, it would be nice to give this a comment saying what it does and what the parameters mean. How does its result relate to the thumbHeight
param? Which of the params are allowed to be undefined? I could figure it out, but I shouldn't have to.
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.
done
@richvdh PTAL - a few bits aren't resolved yet but are pending more feedback. |
So I think remaining things here are:
|
…s PR review request
…on vdh's PR feedback
@richvdh done; feels much nicer to me. PTAL |
@@ -405,7 +405,7 @@ module.exports = React.createClass({ | |||
var scrollPanel = this.refs.scrollPanel; | |||
if (scrollPanel) { | |||
scrollPanel.checkScroll(); |
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.
you don't need to do checkScroll as well as forceUpdate.
@richvdh PTAL again? |
lgtm |
This is part of a set of PRs spanning vector-web, matrix-react-sdk, matrix-js-sdk and synapse.
See also element-hq/element-web#1343