Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Added badge to non-autoplay GIFs #2235

Merged
merged 3 commits into from
Nov 16, 2018
Merged

Added badge to non-autoplay GIFs #2235

merged 3 commits into from
Nov 16, 2018

Conversation

HalleyStarbun
Copy link
Contributor

@HalleyStarbun HalleyStarbun commented Oct 22, 2018

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 [email protected]

Copy link
Member

@turt2live turt2live left a 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 protected]>.

@@ -278,6 +278,7 @@ export default class MImageBody extends React.Component {

let img = null;
let placeholder = null;
let giflabel = null;
Copy link
Member

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 {
Copy link
Member

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)

Copy link
Contributor

@Half-Shot Half-Shot left a 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>;
Copy link
Contributor

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>?

Copy link
Member

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 ?

Copy link
Contributor Author

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.

@turt2live turt2live self-assigned this Nov 15, 2018
@turt2live turt2live merged commit 7dd610b into matrix-org:develop Nov 16, 2018
@dbkr
Copy link
Member

dbkr commented Jan 3, 2019

Was this tested on the light theme? It's illegible.
screenshot 2019-01-03 at 19 00 36

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants