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

Bookmark card: Better url handling and error message #11212

Closed
rishabhgrg opened this issue Oct 8, 2019 · 7 comments
Closed

Bookmark card: Better url handling and error message #11212

rishabhgrg opened this issue Oct 8, 2019 · 7 comments
Labels
good first issue [triage] Start here if you've never contributed before. help wanted [triage] Ideal issues for contributors to help with

Comments

@rishabhgrg
Copy link
Contributor

rishabhgrg commented Oct 8, 2019

Issue Summary

Bookmark card has few minor issues atm as listed below -

  • Better error message handling: Currently bookmark card fails with Unable to parse URL error in case of all failures including insufficient metadata available. We should differentiate between "unable to parse" and "page has insufficient metadata" cases and show appropriate error message to avoid confusion (ref: Bookmark card fails to parse dev.to link #11201)

  • Storing original url on payload: We currently store the url grabbed from page's metadata in the bookmark card payload, which might not be pointing to original pasted url in many cases. We should improve our handling of this by storing the url that is pasted in rather than one that is grabbed from the page. (ref: bookmark card has problem get full of the canonical URL from a page #11209)

  • Fixing empty payload for invalid cases: In case of invalid bookmark card, we currently store payload as {}, unlike embed card which stores original url as part of payload - payload: {url: '....'}. Bookmark card payload needs to be updated to mimic same behavior because it lets the user click the retry/paste as link buttons to get their original url back.

@rishabhgrg rishabhgrg added the good first issue [triage] Start here if you've never contributed before. label Oct 8, 2019
@rishabhgrg rishabhgrg added the help wanted [triage] Ideal issues for contributors to help with label Oct 8, 2019
@jenlky
Copy link

jenlky commented Oct 14, 2019

Hi I would like to work on this issue. The guide to set up local development copy of Ghost at https://ghost.org/docs/install/source/ seems outdated. I'm unable to do git clone --recurse-submodules git@github.com:TryGhost/Ghost && cd Ghost.

@souravdasslg
Copy link

Hey @rishabhgrg , is this issue taken up by anybody ? I'd like to take this up :)

@kamranayub
Copy link

Oh wow, the second bullet point is huge if someone's trying to showcase affiliate-based links. Yikes!

I also noticed that even for relatively large sites like Amazon, you don't seem to get the right image previews and information.

For example, if I want to feature some products I'm reviewing/using, I get this:

image

But if posted to Facebook, the open graph meta tags seem correctly interpreted:

https://developers.facebook.com/tools/debug/sharing/?q=https%3A%2F%2Fwww.amazon.com%2FSmart-Enabled-Google-Assistant-HomeKit%2Fdp%2FB076J8SLSF

image

I love this new feature but especially preserving what I pasted in is critical. It would be ideal to be able to have a fly-out to customize the fields if it failed to parse or doesn't look right, to fix issues like that, too.

@stale
Copy link

stale bot commented Jan 14, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale [triage] Issues that were closed to to lack of traction label Jan 14, 2020
@stale stale bot closed this as completed Jan 21, 2020
@rishabhgrg rishabhgrg reopened this Jan 21, 2020
@stale stale bot removed the stale [triage] Issues that were closed to to lack of traction label Jan 21, 2020
@devaman
Copy link
Contributor

devaman commented Jan 26, 2020

Hi @rishabhgrg , I have tried resolving this issue... Can you take a look at it?

@stale
Copy link

stale bot commented Apr 25, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale [triage] Issues that were closed to to lack of traction label Apr 25, 2020
@rishabhgrg rishabhgrg removed the stale [triage] Issues that were closed to to lack of traction label Apr 30, 2020
kevinansfield added a commit to TryGhost/Admin that referenced this issue Jun 8, 2020
refs TryGhost/Ghost#11212
credit @devaman #1478

- display the original url rather than the fetched url in order to preserve query params and redirects (useful for affiliate links)
kevinansfield added a commit to TryGhost/Koenig that referenced this issue Jun 8, 2020
refs TryGhost/Ghost#11212
credit @devaman TryGhost/Ghost#11542

- use `payload.url` which is the originally entered url rather than `payload.metadata.url` which is the final url after redirects and metascraper extraction
- retains query params and redirects which are useful for things like affiliate links
kevinansfield added a commit that referenced this issue Jun 8, 2020
refs #11212
credit @devaman #11542

- use `payload.url` for the `href` which is the originally entered url rather than `payload.metadata.url` which is the final url after redirects and metascraper extraction
- retains query params and redirects which are useful for things like affiliate links
@kevinansfield
Copy link
Member

kevinansfield commented Jun 8, 2020

I also noticed that even for relatively large sites like Amazon, you don't seem to get the right image previews and information.

Unfortunately Amazon make it difficult to reliably extract product images from their pages. They don't include any of the standardised opengraph data so it's necessary to parse the html and find an element containing the image that Amazon is showing as the primary image - this is easier said than done because they use different html and classes across different types of product page (even for the same product in my testing).

There's an upstream issue on the library we use to extract metadata for bookmark cards here RE extracting Amazon images microlinkhq/metascraper#50. I tested adding the metascraper-amazon dependency to our metascraper plugins but it didn't improve anything.

kevinansfield added a commit that referenced this issue Jun 8, 2020
refs #11212

- if a bookmark card fetch is performed (either directly or from fallback) and the page does not have an extractable title, return a more specific error message than "No provider found for supplied URL."
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue [triage] Start here if you've never contributed before. help wanted [triage] Ideal issues for contributors to help with
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants