Skip to content
This repository has been archived by the owner on Aug 10, 2023. It is now read-only.

Make sticker picker mobile friendlier and fix sending from Riot Mobile #296

Closed
wants to merge 2 commits into from

Conversation

TheTimeWalker
Copy link
Contributor

This PR fixes a few issues for making the sticker picker more usable on Riot Mobile apps.

The sticker picker itself changes the title size and stickers to be more tapable on the mobile screen. There's currently a media rule because the WebView for Riot Mobile is not DPI aware which has to be worked around. It has also been tested on Riot Desktop and there is no usability issue. The stickers are now arranged in a 5 column layout.

The sendSticker function now sends a body with the sticker's description because Riot Android needs a body for the JSON content or else it throws it away making it fail to finish the event. This makes Dimension stickers finally usable on Android at least.

Related issues:
#59 Mobile app support

@turt2live turt2live self-requested a review July 29, 2019 20:19
Copy link
Owner

@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.

otherwise lgtm, I think. I'll want to verify that it still looks okay on desktop/web before merging, but that can wait a bit.

@@ -27,7 +27,7 @@
<div class="stickers">
<div class="sticker" *ngFor="let sticker of pack.stickers trackById"
(click)="sendSticker(sticker, pack)">
<img [src]="getThumbnailUrl(sticker.thumbnail.mxc, 48, 48)" width="48" height="48" class="image"
<img [src]="getThumbnailUrl(sticker.thumbnail.mxc, 48, 48)" class="image"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't lose the width and height because the media repo is not guaranteed to return the size we requested. It can return (much) larger images.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like that the thumbnails never load as 48x48 but as 240x240 on my end anyway. The CSS guarantees that the image always resizes itself to fit in the sticker square for all screen sizes as setting 48x48 statically made it very small on mobile. I can re-add height/width HTML back, but from trying it out quickly it doesn't have any effect as the CSS already overrides it.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, I'll take a look at this too then.

@turt2live
Copy link
Owner

This causes some rendering problems on the web version, which do need to be fixed:

  • The title text becomes massive
  • The stickers have no padding
  • The stickers pop in while they are loading (the width and height on the image tag fix this by reserving the space before the image loads)

Before (expected):
image

After (this PR):
image

@TheTimeWalker
Copy link
Contributor Author

Thanks for checking out! I'll look further, maybe it's some sort of DPI issue as I had to manually add rulings to make it fit on mobile. This has been tested without the port view fix from another pull request, right?

@turt2live
Copy link
Owner

This was all tested with that viewport fix because it's on the master branch.

@TheTimeWalker
Copy link
Contributor Author

Hey, this has been a while that has been looked at. Since there will be changes to how sticker picker will work in e.g. RiotX and there have been layout changes like the picker selector on the bottom, I'll rather close this one and revisit in a later date with a new branch and pull request

@TheTimeWalker TheTimeWalker deleted the patch-mobile branch June 6, 2020 15:50
@TheTimeWalker TheTimeWalker restored the patch-mobile branch June 6, 2020 15:50
@turt2live
Copy link
Owner

What changes are planned for the sticker picker in RiotX? I haven't heard anything about this, and as far as I'm aware there's no technical possibility for it to change.

@TheTimeWalker
Copy link
Contributor Author

TheTimeWalker commented Jun 6, 2020

Since RiotX is a rewrite it possibly will use a more improved WebView that handles screen sizes and DPI better than how it was on riot-android. This means that the layout potentially looks different even with the new CSS changes I've included in this pull request. Rather than keeping it like this where it's way behind since July 29 and kind of breaks the layout on Riot Desktop, I would just test from current master again and work from there.

Oh and I forgot that it includes a workaround to make it work on riot-android which probably is unnecessary since RiotX v0.22 - but I'll have to look into it

@turt2live
Copy link
Owner

Ah, phew. I thought you meant they were going to replace the sticker picker with some sort of proper UI and was worried how they were going to achieve that.

If it's just a webview change, that's fine.

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.

2 participants