Add loading and error indicators to sticker images #12999
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Jenkins BuildsClick to see older builds (21)
|
|
great, thanks! we could fallback to infura ipfs, but dunno if it's possible |
|
there is onError prop, probably we could change source when error |
|
Accessing Gateway content via Subdomain https://{CID}.ipfs.infura-ipfs.io or Path resolution style https://infura-ipfs.io/ipfs/{CID} is limited to 2 requests/sec. oh no, they didn't have this before :( |
|
we could try dweb.link then, haven't find anything about its rate limiting |
flexsurfer
approved these changes
Jan 13, 2022
79% of end-end tests have passedFailed tests (35)Click to expand
Passed tests (128)Click to expand |
That's a great idea! I'll see how I can abstract this into an |
|
Thanks for the PR, good job! 1. Sticker packs’ main images are not loadedIn nightly and release builds those images are loaded successfully so I guess this is pr issue. Endless loader is spinning. For reproduction just open sticker market and pay attention at Sticker packs’ main images. Reproducing it only for Android (Device Samsung A52, OS Android 11). In IOS build images are loaded successfully. endless_loader.mp4
2. Sticker packs’ main images stuck in loading state (can be observed in video attached above)Expected result: if images are failed to be loaded, loading indicator should be changed by error indicator just like it happens to images inside sticker packs. 3. Empty spaces between stickers in the last row of stickers’ list in some packs.Reproducing both for IOS and Android. For reproduction: open sticker market -> select “Meowy Christmas” pack -> pay attention at blank spaces between stickers at the bottom of the list.
4. react/fast-image element lost it’s accessibility-label :sticker-iconIt causes some auto tests to fail. Expected result: react/fast-image element has accessibility-label :sticker-icon . For image-with-loader element it will be nice to set accessibility-label :sticker-icon-loading
|
msuess
force-pushed
the
feature/stickers-ux-improvements
branch
from
ac3a36e
to
2a3c186
Compare
implemented |
|
@pavloburykh the issues should be fixed now |
@msuess thanks! |
|
@msuess could you please take a look at lint errors during building https://ci.status.im/job/status-react/job/prs/job/android/job/PR-12999/ |
|
builds are ok , its just lint , you need to run make lint and fix formating |
|
@pavloburykh builds can be tested |
0% of end-end tests have passedFailed tests (1)Click to expand
|
0% of end-end tests have passedFailed tests (1)Click to expand
|
0% of end-end tests have passedFailed tests (1)Click to expand
|
0% of end-end tests have passedFailed tests (3)Click to expand
|
|
@msuess thanks for fixing #12999 (comment) I faced a new issue in new Android build, please take a look (For IOS it is ok). Stickers in most of stickerpacks are broken. See screenshots and video below. STRs:
Actual result: sticker images are broken telegram-cloud-document-2-5420505957438330332.mp4
|
msuess
force-pushed
the
feature/stickers-ux-improvements
branch
from
e9e64af
to
21da153
Compare
96% of end-end tests have passedFailed tests (3)Click to expand
Passed tests (76)Click to expand
|
|
@msuess thanx for fixes. PR is ready for merge. |
|
@msuess could you please squash commits, and I think we need to review PR again, because there were changes ? |
flexsurfer
requested changes
Mar 22, 2022
flexsurfer
reviewed
Mar 22, 2022
| (rest @current-source))) | ||
| :onLoad #(reset! loaded? true) | ||
| :style (if @loaded? style {}) | ||
| :source (first @current-source) |
There was a problem hiding this comment.
what if all sources failed ?
There was a problem hiding this comment.
An error icon will be displayed if all sources fail
|
moving back to Test because additional changes were made. Waiting for changes to be reviewed and approved and then will proceed with re-test. |
flexsurfer
approved these changes
Mar 25, 2022
97% of end-end tests have passedFailed tests (2)Click to expand
Passed tests (77)Click to expand
|
94% of end-end tests have passedFailed tests (5)Click to expand
Passed tests (74)Click to expand
|
✔️ status-react/prs/ios/PR-12999#10 🔹 ~21 min 🔹 262faf8 🔹 📦 ios package |
|
re-tested. Ready for merge. |
|
@msuess could you squash commits ? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds loading spinners and an error indicator to sticker images. While it doesn't fix #12770, it should at least provide a better experience than just showing nothing.
stickers-loaders.mov
status: ready