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
Create an oEmbed endpoint for maps #1526
Conversation
22b1725
to
d2ce895
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent!
Shame on me that I did not implement this earlier!
@@ -675,3 +675,63 @@ def test_download_my_map(client, map, datalayer): | |||
# Test response is a json | |||
j = json.loads(response.content.decode()) | |||
assert j["type"] == "umap" | |||
|
|||
|
|||
@pytest.mark.parametrize("share_status", [Map.PRIVATE, Map.BLOCKED, Map.OPEN]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about the OPEN
here. The purpose of the OPEN
status, is that a map is not discoverable, but once you have a link you can view it normally, so why not allowing also the oEmbed in this case ?
j = json.loads(response.content.decode()) | ||
assert j["type"] == "rich" | ||
assert j["version"] == "1.0" | ||
assert j["width"] == 800 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, are those dimensions configurable by the caller ? Or should we have a relative width ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The spec says:
The width/height in pixels required to display the HTML.
And maybe it was before we can have more flexible dimensions?
format_ = request.GET.get("format", "json") | ||
if format_ != "json": | ||
response = HttpResponseServerError("Only `json` format is implemented.") | ||
response.status_code = 501 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not a 40x here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to stick to the spec as much as possible:
501 Not Implemented
The provider cannot return a response in the requested format. This should be sent when (for example) the request includesformat=xml
and the provider doesn't support XML responses. However, providers are encouraged to support both JSON and XML.
data["height"] = height | ||
width = 800 | ||
data["width"] = width | ||
# TODISCUSS: do we keep width=100% by default for the iframe? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we keep both width in relative ?
For the width, i give you 3 exemples : PeerTube, Youtube and osm.org
So 100% seems like a good choice |
@bristow thank you so much for these example! Do you also have the raw responses from the oembed endpoints of these services to see how they deal with the |
Here is the response from the PeerTube developer The oembed payload is generated here: https://github.com/Chocobozzz/PeerTube/blob/develop/server/core/controllers/services.ts#L99 For example: https://peertube2.cpy.re/services/oembed?url=https%3A%2F%2Fpeertube2.cpy.re%2Fw%2F1zywKcr1ChzL7R9rG6yCnq Another example where we limit the size: https://peertube2.cpy.re/services/oembed?url=https%3A%2F%2Fpeertube2.cpy.re%2Fw%2F1zywKcr1ChzL7R9rG6yCnq&maxwidth=200 I hope this helps |
Definitely! I'm curious though because if you take the payload from https://peertube2.cpy.re/services/oembed?url=https%3A%2F%2Fpeertube2.cpy.re%2Fw%2F1zywKcr1ChzL7R9rG6yCnq
Which does not states |
I took these codes from the source code of this site, which offers remote media integration via oEmbed. |
Will this feature be deployed in the near future? I'm willing to test it on a pre-prod platform if need be. Thank you! |
Very soon! We are in a Javascript refactor which prevents us to add more features to the deployment flow, but it seems we are close to getting back to normal life! Sorry for that :) |
@bristow this is now deployed on the OSM instance 🤞 |
I analysed the source code of an umap map. Perhaps a clue... |
Fix #162
Based on https://oembed.com/