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

Fixes: #360, #534, #690, #716: Display default image in case of broken image URL response from twitter servers #742

Closed
wants to merge 1 commit into from

Conversation

simsausaurabh
Copy link
Member

@simsausaurabh simsausaurabh commented Jun 6, 2018

Changes proposed in this pull request

  • Added required default image(with corresponding resolutions).
  • Made Changes to display default image when the image Url response from twitter image servers is broken.

The URLs returned by api.loklak won't work for sometimes, although they are originally extracted from twitter. But the problem is with twitter image servers, which do not allow to access a lot of images and sometimes none. For more information please see https://twittercommunity.com/t/original-profile-image-not-found/67046. Until a reasonable solution is found on twitter server side, a default image should be placed inside of empty broken image containers with required resolution of the container.

This patch adds default image on broken url response in user-info-box in sidebar(To replace Profile banner image, Profile images of twitter accounts i.e. user, following, and followers).

Screenshots (if appropriate)
Will provide once the api.loklak starts again, currently its down.

Link to live demo: http://pr-742-fossasia-loklaksearch.surge.sh
Parent issue #534, #716
Closes #360 #690

…y default image in case of broken image URL response from twitter
@codecov
Copy link

codecov bot commented Jun 6, 2018

Codecov Report

Merging #742 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #742   +/-   ##
=======================================
  Coverage   53.41%   53.41%           
=======================================
  Files         107      107           
  Lines        2881     2881           
  Branches      374      374           
=======================================
  Hits         1539     1539           
  Misses       1218     1218           
  Partials      124      124

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7eaf03f...dd7441e. Read the comment docs.

@simsausaurabh
Copy link
Member Author

@singhpratyush @hemantjadon @praveenojha33 please review.

@praveenojha33
Copy link
Member

Unable to open link even in firefox and chrome incognito
image

@simsausaurabh
Copy link
Member Author

@praveenojha33 Please try this link: http://loud-relation.surge.sh

@praveenojha33
Copy link
Member

@simsausaurabh The problem persist with this link as well
image
I think it is a temporary problem, I should review it after sometime.

@praveenojha33
Copy link
Member

No results are coming. Is loklak server down?
image

@simsausaurabh
Copy link
Member Author

@praveenojha33 Results are coming, please try again!

Copy link
Member

@praveenojha33 praveenojha33 left a comment

Choose a reason for hiding this comment

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

Hello @simsausaurabh When I made a query from:simsausaurabh on your deployed PR and on loklak.org I got this result
screenshot from 2018-06-07 14-33-54
image

screenshot from 2018-06-07 14-34-39
screenshot from 2018-06-07 14-34-58

As you can see some images are available but for that also default image is shown. Please look into it.

@simsausaurabh
Copy link
Member Author

@praveenojha33 The count of request response is more on hosted website i.e. Loklak.org. The test PR is deployed on surge and its response capacity is less in comparison to loklak.org. That is why whenever you will search from loklak.org, you will get less broken results in comparision to your local deployment. Its a problem with the surge, which is not able to fetch/hold that match response. And the patch correctly displays default image when there is unavailable image url from twitter. This won't be a problem once the patch gets merged, you will see less default images and more twitter images.

@praveenojha33
Copy link
Member

praveenojha33 commented Jun 7, 2018

Hello @simsausaurabh Now I have compared it with a surge link of previous PR and surge link for this PR . I get this result for surge link of PR 730
image
And for this PR:
image

</div>
<div class="profile-image">
<img src="{{ apiResponseUser.profile_image_url }}"/>
<img src="{{ apiResponseUser.profile_image_url }}" onError="this.src='./assets/images/def65.jpg';"/>
Copy link
Member Author

Choose a reason for hiding this comment

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

@praveenojha33 The default image gets load only when there is any error(like broken URL). I have not replaced the original response url. As mentioned in the PR description, the problem is with twitter image servers, which do not allow to access a lot of images and sometimes none. And it also depends on host's response capacity.

Copy link
Member

@praveenojha33 praveenojha33 left a comment

Choose a reason for hiding this comment

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

OK, Lets see how it works on Loklak.org

@mariobehling
Copy link
Member

I am not sure, if this is the best approach, cause it does not start to path a way to solve the issue. A better approach would seem to me to implement the solution on the server itself.

@simsausaurabh
Copy link
Member Author

I looked into the current available solutions. But still I'll try to work on to resolve this on server side. Thanks @mariobehling 👍

@mariobehling
Copy link
Member

Why are you committing this to master? Commit to dev, please.

@simsausaurabh simsausaurabh deleted the fixImage branch June 13, 2018 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants