-
-
Notifications
You must be signed in to change notification settings - Fork 833
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.
This looks pretty good. Thanks for implementing it. I've noticed a couple very small things, but otherwise I think this is probably good to go. Also don't forget to sign off on the changes by leaving a comment on the PR with Signed-off-by: Your Name <email@example.org>
.
@@ -278,6 +278,7 @@ export default class MImageBody extends React.Component { | |||
|
|||
let img = null; | |||
let placeholder = null; | |||
let giflabel = null; |
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.
nit: gifLabel
(capital L)
@@ -46,3 +46,14 @@ limitations under the License. | |||
.mx_MImageBody_thumbnail_spinner > * { | |||
transform: translate(-50%, -50%); | |||
} | |||
|
|||
.mx_MImageBody_GIFlabel { |
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.
nit: GIFLabel
or GifLabel
(capital L)
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.
LGTM apart from tiny nit
@@ -302,11 +303,14 @@ export default class MImageBody extends React.Component { | |||
onMouseLeave={this.onImageLeave} />; | |||
} | |||
|
|||
if (this._isGif() && !SettingsStore.getValue("autoplayGifsAndVideos") && !this.state.hover) { | |||
gifLabel = <p className="mx_MImageBody_gifLabel">GIF</p>; |
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.
Tiny nit, <p>
vs <span>
?
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 span
is probably better semantically, although p
gives you a bunch of CSS/positioning for free. What do you think @MaxwellKepler ?
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.
Apologies for the delay. I personally don't see any problem with <p>
, it seems to work fine and I don't see it causing any problems in future.
Added a small badge over non-autoplaying GIFs that vanishes when moused over / played.
Made in response to: element-hq/element-web#7344
Signed-off-by: Kieran Gould starboundshep@gmail.com