Skip to content
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.

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

Weird image failed rendering in thread body #4812

Closed
brianlovin opened this issue Mar 4, 2019 · 4 comments

Comments

Projects
3 participants

@spectrum-bot spectrum-bot bot added the Bug label Mar 4, 2019

@mxstbr mxstbr added this to Backlog in Planning via automation Mar 5, 2019

@aszx87410

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

Hi, I think it's something to do with margin collapsing.

When there is an error when rendering image, that's say image not found, the img's height will become 0 and therefore margin doesn't work. So you will see the screenshot below:

螢幕快照 2019-03-21 上午11 17 58

There is a simple solution came to my mind, add alt attribute should solve the problem. Because if there is any error when rendering image, alt text will appear so that img still get height. Like the screenshot below:

螢幕快照 2019-03-21 上午11 17 40

For alt attribute, it can be something like error, Image not found or just put the image url as alt. What do you think? @brandly @mxstbr

I can submit a PR for it, happy to contribute to spectrum 😄

@brianlovin

This comment has been minimized.

Copy link
Member Author

commented Mar 21, 2019

@aszx87410 this is brilliant - ideally we never break an image, but if one does fail this seems like a more graceful regression than what we currently have. Let's go for it!

@aszx87410

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

@brianlovin I found that we already render alt attribute for image:

) => <img key={key} src={data.src} alt={data.alt} />,

So it's like an edge case, author embed images without providing alt text. The normal image is like this ![img.png](img_url) and this one causes the issue: ![](img_url) because there is no text.

I think we can just add a default value in case there is no alt text, like alt={data.alt || 'Image'}, is it okay? I will submit a PR if it's fine.

@aszx87410 aszx87410 referenced this issue Mar 21, 2019

Merged

Add default alt text to img #4887

1 of 3 tasks complete
@aszx87410

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

PR created, let me know if there is any issues, thanks. #4887

Planning automation moved this from Backlog to Done Mar 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.