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

Fixes for top_info avatars #4376

Closed
wants to merge 1 commit into
base: release-2.1
from

Conversation

Projects
None yet
5 participants
@Gwenwyfar
Collaborator

Gwenwyfar commented Oct 28, 2017

Fixes image scaling and #4343.

This is how it will display on images that fail to load:
broken-image

On some non-supporting browsers, nothing will show (the alt text will also remain hidden, to not break the layout).

Image used is my own.

Fixes for alert avatars
Fixes scaling and missing images display.

Signed-off-by: Gwenwyfar <gwenwyfar@protonmail.com>
@Antes

This comment has been minimized.

Show comment
Hide comment
@Antes

Antes Oct 29, 2017

Contributor

I don't think adding yet another default (fallback) avatar is the solution, core system should be loading a default avatar when one does not exist.

Contributor

Antes commented Oct 29, 2017

I don't think adding yet another default (fallback) avatar is the solution, core system should be loading a default avatar when one does not exist.

@Gwenwyfar

This comment has been minimized.

Show comment
Hide comment
@Gwenwyfar

Gwenwyfar Oct 29, 2017

Collaborator

If the theme images folder is broken then the entire theme will be, not just avatars as is the case here. We could use the same image as avatarless members, but that would become hard to differentiate from broken avatars.

Collaborator

Gwenwyfar commented Oct 29, 2017

If the theme images folder is broken then the entire theme will be, not just avatars as is the case here. We could use the same image as avatarless members, but that would become hard to differentiate from broken avatars.

@Antes

This comment has been minimized.

Show comment
Hide comment
@Antes

Antes Oct 29, 2017

Contributor

If the image folder is broken, user needs to fix bigger problem. Such additions only required if there is no fallback to current solution.

Contributor

Antes commented Oct 29, 2017

If the image folder is broken, user needs to fix bigger problem. Such additions only required if there is no fallback to current solution.

@Gwenwyfar

This comment has been minimized.

Show comment
Hide comment
@Gwenwyfar

Gwenwyfar Oct 29, 2017

Collaborator

If this can be fixed another way then whoever can do that feel free to do so.

Collaborator

Gwenwyfar commented Oct 29, 2017

If this can be fixed another way then whoever can do that feel free to do so.

@jdarwood007

This comment has been minimized.

Show comment
Hide comment
@jdarwood007

jdarwood007 Oct 29, 2017

Member

Problem would be css does not know the avatars location, since that url is separate from the themes url.

Member

jdarwood007 commented Oct 29, 2017

Problem would be css does not know the avatars location, since that url is separate from the themes url.

@Antes

This comment has been minimized.

Show comment
Hide comment
@Antes

Antes Oct 29, 2017

Contributor

@Gwenwyfar there shouldn't be any need of any fix, the broken (non-existent etc...) avatars should fallback to default avatar, if default avatar does not exist in the situation that means either code lacks the fallback to default avatar or setup has issues related to locating the default avatar. What you are offering is actually a fallback for default avatar.

Contributor

Antes commented Oct 29, 2017

@Gwenwyfar there shouldn't be any need of any fix, the broken (non-existent etc...) avatars should fallback to default avatar, if default avatar does not exist in the situation that means either code lacks the fallback to default avatar or setup has issues related to locating the default avatar. What you are offering is actually a fallback for default avatar.

@Gwenwyfar

This comment has been minimized.

Show comment
Hide comment
@Gwenwyfar

Gwenwyfar Oct 29, 2017

Collaborator

Exactly. If the problem can be fixed with the default avatar fallback, then we don't need this fix.

I made this assuming the paths to avatars don't work (for whatever reason) and that this may be a common situation, but if the default avatar is already serving that purpose there is no need for this, if it can be made to work correctly in all cases (although keeping the transparent alt doesn't hurt in any case)

Collaborator

Gwenwyfar commented Oct 29, 2017

Exactly. If the problem can be fixed with the default avatar fallback, then we don't need this fix.

I made this assuming the paths to avatars don't work (for whatever reason) and that this may be a common situation, but if the default avatar is already serving that purpose there is no need for this, if it can be made to work correctly in all cases (although keeping the transparent alt doesn't hurt in any case)

@colinschoen

This comment has been minimized.

Show comment
Hide comment
@colinschoen

colinschoen Nov 2, 2017

Member

I agree with @Antes. If we are displaying this second-fallback image something is seriously wrong and it may create more confusion as to why or where this is actually loading from if all the paths are wrong.

Member

colinschoen commented Nov 2, 2017

I agree with @Antes. If we are displaying this second-fallback image something is seriously wrong and it may create more confusion as to why or where this is actually loading from if all the paths are wrong.

@colinschoen

This comment has been minimized.

Show comment
Hide comment
@colinschoen

colinschoen Dec 5, 2017

Member

Closing per rationale above.

Member

colinschoen commented Dec 5, 2017

Closing per rationale above.

@colinschoen colinschoen closed this Dec 5, 2017

@Gwenwyfar

This comment has been minimized.

Show comment
Hide comment
@Gwenwyfar

Gwenwyfar Dec 5, 2017

Collaborator

Ok. I'll submit the other fixes included in this in another PR.

Collaborator

Gwenwyfar commented Dec 5, 2017

Ok. I'll submit the other fixes included in this in another PR.

@Gwenwyfar Gwenwyfar deleted the Gwenwyfar:avatars branch Jan 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment