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

Use resolve for OG metadata #6787

Merged
merged 7 commits into from
Aug 7, 2021
Merged

Use resolve for OG metadata #6787

merged 7 commits into from
Aug 7, 2021

Conversation

btzr-io
Copy link
Collaborator

@btzr-io btzr-io commented Aug 5, 2021

PR Checklist

Please check all that apply to this PR using "x":

  • I have checked that this PR is not a duplicate of an existing PR (open, closed or merged)
  • I added a line describing my change to CHANGELOG.md
  • I have checked that this PR does not introduce a breaking change
  • This PR introduces breaking changes and I have provided a detailed explanation below

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting)
  • Refactoring (no functional changes)
  • Documentation changes
  • Other - Please describe:

Fixes

Closes #6780
Closes #6464
Closes #6812

@btzr-io
Copy link
Collaborator Author

btzr-io commented Aug 5, 2021

I'm still not sure how to test this...

@infinite-persistence
Copy link
Contributor

  • yarn && yarn dev:web-server
  • localhost:1337 in the browser after build is done.
  • Open a content page, then F5 to reload (or enter URL directly). The OG is not populated on link navigation.
  • Verify the <head> for the OG and GoogleVideo info

@btzr-io
Copy link
Collaborator Author

btzr-io commented Aug 5, 2021

* `yarn && yarn dev:web-server`

* `localhost:1337` in the browser after build is done.

* Open a content page, then F5 to reload (or enter URL directly).  The OG is not populated on link navigation.

* Verify the `<head>` for the OG and GoogleVideo info
  
  * <img alt="" width="400" src="https://user-images.githubusercontent.com/64950861/128299681-e2f50acd-2d37-483f-8ca1-979feb89f810.png">

Nice thanks ✌️

web/src/html.js Outdated Show resolved Hide resolved
web/src/html.js Outdated
const thumbnail = value && value.thumbnail && value.thumbnail.url;
const mediaType = source && source.media_type;
const mediaDuration = media && media.duration;
const creationTimestamp = meta && (meta.creation_timestamp || 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should do value.release_time --> creation time --> 0. We've been using release time, which works for newer claims.

@infinite-persistence
Copy link
Contributor

infinite-persistence commented Aug 6, 2021

  • Nice, seems like "keyword"/tags now work too. I'm guessing CQ wasn't returning tags all this while.
    • image
  • Channel Page info is better now too. Previously, the link is totally wrong.
    • -->

@@ -18,6 +18,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
- Improve twitter share _community pr!_ ([#6690](https://github.com/lbryio/lbry-desktop/pull/6690))
- Update lighthouse search api _community pr!_ ([#6731](https://github.com/lbryio/lbry-desktop/pull/6731))
- Update sockety api _community pr!_ ([#6747](https://github.com/lbryio/lbry-desktop/pull/6747))
- Use resolve for OG metadata instead of chainquery _community pr!_ ([#6787](https://github.com/lbryio/lbry-desktop/pull/6787))
Copy link
Contributor

@infinite-persistence infinite-persistence Aug 6, 2021

Choose a reason for hiding this comment

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

Not sure where the error is, but Embed OG's aren't showing up (it's showing the fallback).
Example embed: https://odysee.com/$/embed/eevblog-1412-argon-ion-laser-teardown!/1fe325659bf8d791f72e02bb614a942fa4b571bf?r=5yqd2A3cQ59DhkNRYTJq3BApzdcr7KSn

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The embed url doesn't use : or # to separate the id.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Adding this .replace('\/', '#'); fixes the issue. Not sure if this will work on all cases.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As long as embed url is always $/embed/name/id ^^

@btzr-io
Copy link
Collaborator Author

btzr-io commented Aug 6, 2021

Is there any other urls that uses / instead of : or # before the claim id ?

@infinite-persistence
Copy link
Contributor

infinite-persistence commented Aug 7, 2021

Is there any other urls that uses / instead of : or # before the claim id ?

Tom mentioned that anywhere we use generateStreamURL, generateDownloadUrl and generateDirectUrl. But those doesn't impact the header, I think.

However, I noticed that http://odysee.com/eevblog-1412-argon-ion-laser-teardown!/1fe325659bf8d791f72e02bb614a942fa4b571bf redirects and shows the right header, but the PR version does not.

@btzr-io
Copy link
Collaborator Author

btzr-io commented Aug 7, 2021

However, I noticed that http://odysee.com/eevblog-1412-argon-ion-laser-teardown!/1fe325659bf8d791f72e02bb614a942fa4b571bf redirects and shows the right header, but the PR version does not.

Same issue with embed urls. parseURI fails to detect the claim id, because there is no '#'

const { isChannel, streamName, channelName, channelClaimId, streamClaimId } = parseURI(url);

@btzr-io
Copy link
Collaborator Author

btzr-io commented Aug 7, 2021

http://odysee.com/eevblog-1412-argon-ion-laser-teardown!/1fe325659bf8d791f72e02bb614a942fa4b571bf

This is actually the equivalent of:

lbry://eevblog-1412-argon-ion-laser-teardown!

Instead of:

lbry://eevblog-1412-argon-ion-laser-teardown!#1fe325659bf8d791f72e02bb614a942fa4b571bf

See: #6812

@btzr-io
Copy link
Collaborator Author

btzr-io commented Aug 7, 2021

However, I noticed that http://odysee.com/eevblog-1412-argon-ion-laser-teardown!/1fe325659bf8d791f72e02bb614a942fa4b571bf redirects and shows the right header, but the PR version does not.

I don't think the redirection issue has nothing todo with the changes on this pull request.

@infinite-persistence infinite-persistence deleted the fix-og-meta branch August 7, 2021 11:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants