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.

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

Bookmark card issue resolved #11542

Closed
wants to merge 1 commit into from
Closed

Bookmark card issue resolved #11542

wants to merge 1 commit into from

Conversation

devaman
Copy link
Contributor

@devaman devaman commented Jan 26, 2020

Bookmark card: Better url handling and error message resolves

refs/closes #11212

  • Better error message handling
  • Storing original url on payload
  • Fixing empty payload for invalid cases:

Check Ghost Admin pull request for Bookmark card issue also. Both of them together resolves this issue.

@rishabhgrg
Copy link
Contributor

👋 Hey @devaman, thanks for the PRs. I'll take a look at it this week, meanwhile can you please rebase on master and resolve conflicts so the tests can run as well? 😄

@devaman
Copy link
Contributor Author

devaman commented Feb 10, 2020

done @rishabhgrg

@kevinansfield
Copy link
Member

Heads up that I force-pushed this to clean up the submodules and lockfile that shouldn't have been included.

I haven't merged yet as I have a concern about old posts. We need to double-check to make sure we've always stored the top-level url value in the payload otherwise this PR will break old content any time it's re-rendered to html. If we have always stored that value then this PR is 👍 otherwise it will need updating to have a fallback behaviour with payload.url || payload.metadata.url

kevinansfield added a commit to TryGhost/Koenig that referenced this pull request Jun 8, 2020
@kevinansfield
🐛 Fixed original url being lost when rendering bookmark cards
b9844bd 
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 pull request Jun 8, 2020
@kevinansfield
🐛 Fixed original url/query params not being used in bookmark cards
e1fd8cc 
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

Thanks @devaman! The original-url fix has been implemented following most of this PR but because the relevant code has been extracted to an external repo this PR wasn't usable directly.

I've added you as a credit in the relevant commits
TryGhost/Koenig@b9844bd
e1fd8cc

@devaman
Copy link
Contributor Author

devaman commented Jun 8, 2020

Thanks @kevinansfield 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bookmark card: Better url handling and error message
3 participants
@devaman @rishabhgrg @kevinansfield