Skip to content

🩹 (#2463): show a fallback image when image can't be loaded #2466

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
juliusknorr merged 11 commits into from
Jun 9, 2022
Merged

🩹 (#2463): show a fallback image when image can't be loaded #2466

juliusknorr merged 11 commits into from
Jun 9, 2022

Conversation

@vinicius73
Copy link
Member

@vinicius73 vinicius73 commented Jun 1, 2022 •
edited
Loading

Summary

Display a fallback image when image loading fails.

image

@vinicius73 vinicius73 added design Experience, interaction, interface, … feature: formatting Features related to text formatting and node types 3. to review labels Jun 1, 2022
@vinicius73
Copy link
Member Author

vinicius73 commented Jun 1, 2022

/compile

@vinicius73 vinicius73 changed the title 🩹 (#2463): show a falback image when image can't be loaded 🩹 (#2463): show a fallback image when image can't be loaded Jun 1, 2022
jancborchardt
jancborchardt previously requested changes Jun 2, 2022
View reviewed changes
Copy link
Member

@jancborchardt jancborchardt left a comment •
edited by vinicius73
Loading

Choose a reason for hiding this comment

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

@vinicius73 some design feedback:

  • The icon can be the general image icon, does not need to be a specific "broken" one: https://fonts.google.com/icons?selected=Material%20Icons%3Aimage%3A
  • Color should be grey, also for the text, color-text-maxcontrast. It’s not a "red error" type big deal. :)
  • The message "Failed to load image" can show below together with the filename, as to not introduce another text block above. Just: "Failed to load [filename]" like "Failed to load image2.jpg"
  • The initial / should be removed, it’s too technical

@vinicius73
Copy link
Member Author

vinicius73 commented Jun 2, 2022

The initial / should be removed, it’s too technical

It's the description of the image, I've put it as an example.

julien-nc
julien-nc reviewed Jun 3, 2022
View reviewed changes
Copy link
Member

@julien-nc julien-nc left a comment

Choose a reason for hiding this comment

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

I agree with @jancborchardt comments.

Code-wise, nothing to add. I guess the ErrorLoadImage is part of the solution we figured out with @mejo- about knowing when all images are loaded in Collectives, right?

The error could be renamed to LoadImageError but that's nitpicking 😁.

@vinicius73
Copy link
Member Author

vinicius73 commented Jun 6, 2022

@jancborchardt what is the best alternative?

Original/raw src parameter. (for now I am using this)
image

With full URL used to load image (parsed by ImageView component)
image

@vinicius73
Copy link
Member Author

vinicius73 commented Jun 6, 2022

/compile

@mejo-
Copy link
Member

mejo- commented Jun 6, 2022 •
edited
Loading

@jancborchardt what is the best alternative?

Original/raw src parameter. (for now I am using this) image

I would vote for Failed to load invalid.png, without square brackets and without any path elements - just the basename of the image. That's the least technical and easiest to understand for users in my opinion.

mejo-
mejo- reviewed Jun 6, 2022
View reviewed changes
Copy link
Member

@mejo- mejo- left a comment

Choose a reason for hiding this comment

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

Thanks a lot for implementing this, @vinicius73 <3

@vinicius73 vinicius73 force-pushed the 2463-load-fallback-images-when-image-could-not-be-loaded branch from 3ae1576 to f578231 Compare June 8, 2022 19:25
@vinicius73
Copy link
Member Author

vinicius73 commented Jun 8, 2022

/compile

@vinicius73 vinicius73 requested a review from max-nextcloud June 8, 2022 21:36
@vinicius73
Copy link
Member Author

vinicius73 commented Jun 8, 2022

/compile

@vinicius73
Copy link
Member Author

vinicius73 commented Jun 8, 2022

/compile amend

@nextcloud-command nextcloud-command force-pushed the 2463-load-fallback-images-when-image-could-not-be-loaded branch from 1f879a1 to 8f3b574 Compare June 8, 2022 21:58
@vinicius73
Copy link
Member Author

vinicius73 commented Jun 8, 2022

@juliushaertl I've reverted this commit: a70841a

It was generating error 500 from API.

POST /index.php/apps/text/session/fetch HTTP/1.1" 500

Vinicius Reis added 3 commits June 9, 2022 10:35
@juliusknorr
🩹 (#2463): show a falback image when image can't be loaded
f7451b5 
Signed-off-by: Vinicius Reis <[email protected]>
@juliusknorr
🚸 (#2463): improve ux
a54c52d 
change error icon and color and move error message

Signed-off-by: Vinicius Reis <[email protected]>
@juliusknorr
✨ (#2463): emit error when image loading fails
5abdf58 
Signed-off-by: Vinicius Reis <[email protected]>
Vinicius Reis added 7 commits June 9, 2022 10:35
@juliusknorr
♻️ (#2463): remove duplicated code
f2c95d1 
Signed-off-by: Vinicius Reis <[email protected]>
@juliusknorr
🎨 (#2463): indent code
803eb23 
Signed-off-by: Vinicius Reis <[email protected]>
@juliusknorr
✅ (#2463): [WIP] ImageView tests
8da4222 
direct access

Signed-off-by: Vinicius Reis <[email protected]>
@juliusknorr
🎨 (#2463): format code
7429ce8 
Signed-off-by: Vinicius Reis <[email protected]>
@juliusknorr
🐛 (#2463): fix currentDirectory parameter
36e497d 
Signed-off-by: Vinicius Reis <[email protected]>
@juliusknorr
✅ (#2463): add error tests
00d508b 
Signed-off-by: Vinicius Reis <[email protected]>
@juliusknorr
🚸 (#2463): change error message
a114f12 
Signed-off-by: Vinicius Reis <[email protected]>
@juliusknorr juliusknorr force-pushed the 2463-load-fallback-images-when-image-could-not-be-loaded branch from 4f6c948 to a114f12 Compare June 9, 2022 08:36
@juliusknorr
Copy link
Member

juliusknorr commented Jun 9, 2022

Rebased to retrigger tests after #2495 was merged

@juliusknorr
Copy link
Member

juliusknorr commented Jun 9, 2022

/compile

@nextcloud-command
Compile assets
4aadc40 
Signed-off-by: nextcloud-command <[email protected]>
@juliusknorr juliusknorr requested a review from mejo- June 9, 2022 09:28
@juliusknorr juliusknorr merged commit 67c6924 into master Jun 9, 2022
@delete-merged-branch delete-merged-branch bot deleted the 2463-load-fallback-images-when-image-could-not-be-loaded branch June 9, 2022 09:29
@juliusknorr juliusknorr mentioned this pull request Jun 9, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@juliusknorr juliusknorr juliusknorr approved these changes

@azul azul Awaiting requested review from azul

@nimishavijay nimishavijay Awaiting requested review from nimishavijay

@luka-nextcloud luka-nextcloud Awaiting requested review from luka-nextcloud

@jancborchardt jancborchardt Awaiting requested review from jancborchardt

@julien-nc julien-nc Awaiting requested review from julien-nc

@max-nextcloud max-nextcloud Awaiting requested review from max-nextcloud

@mejo- mejo- Awaiting requested review from mejo-

Assignees

No one assigned

Labels

3. to review design Experience, interaction, interface, … feature: formatting Features related to text formatting and node types

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Load fallback images when image could not be loaded

7 participants

@vinicius73 @mejo- @juliusknorr @jancborchardt @julien-nc @nextcloud-command