Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Embed: design doc for new embed API #8039
Embed: design doc for new embed API #8039
Changes from 1 commit
9d4ebe5
fe633ec
66d839e
06c8a97
7448158
1d3c097
1d43423
81edc9c
89a5795
4efd3ec
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
You can send
section=
to get only the section you want.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.
What I mean here is that it always returns all the sections (title and id), not the section content.
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.
If this refers to the
dd
/dt
issue, I'm not sure if this will be easily doable for all the cases. We may end up re-parsing thedd
to convert that HTML tag to another one, but that will break the style based ondd
tags. So, this will be tricky to do it right and make it work for all the themes --which is also related with the newextras
field.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.
We just need to extract only that element. For example
becomes
Extracting content isn't hard to do 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.
Hrm... I think that has the same problem.
When parsing
citation
/glossary
we don't need to return the title/definition (dt
), we only want the description (dd
). Example in codereadthedocs.org/readthedocs/embed/views.py
Lines 329 to 335 in eb682d8
Following, your example, to make it valid it should be:
However, this won't probably render correctly inside the tooltip, since in that context is not a description list --it's not a list at all-- but just the definition of one of the term from that list.
In those cases,
citation
/glossary
will be rendered in the same than any other definition list and I'm not sure we want that. We need to think how we will support this. Example:Glossary (only includes the definition)
Config value (includes the definition list completely repeating the term hovered)
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 think we should repeat the title, it doesn't necessary match the text where the tooltip is present. Removing the title if it's the same as the text can be done in the client side.
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.
Glossary and Citation special cases can be managed from the frontend/client since we have all the data we need (note that we return the
.next()
HTML element). We will just need to parse it remove the content we don't want to display.However, for the
dt
case (definition of a config, Python method/function, etc) it won't work because we need the.parent()
of HTML element picked by ID. Take a look at this example:(example from https://sphinx-hoverxref.readthedocs.io/en/latest/configuration.html#confval-hoverxref_role_types)
With the id
confval-hoverxref_role_types
we are currently returning the wholedl
because we access to the.parent()
element. If we return just the HTML tag matched, we will be returning only thedt
which does not include the description nor any content.Without handling this special case properly, we will show tooltips like:
We need to find a general solution for these special cases.
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.
@humitos not sure to understand the problem there? that's the same example we have in the document. The id is on the dt element, so we fetch the dt and the next dd enclosed in the dl tag.
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 may not have expressed myself properly.
This is a special case because we are manipulating the HTML to manage a particular case on Sphinx: return the parent of a particular ID, instead of itself. Since we are going to support external sites that will not be generated with Sphinx (*), we can't assume that we always have to do this special HTML manipulation. That's why I'm saying that we need a general solution to differentiate these special cases from regular (non-Sphinx) pages.
I'm thinking that we will need to communicate this to the backend somehow. Maybe a request GET argument:
?doctool=sphinx&version=3.5.1
(*) or even with a different version of Sphinx that may adds the
id=
in thedl
.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.
that's part of how we expect the html to be, following semantic html and using correct tags, if a site has malformed tags or if the id is in a different element we don't try to fix it, we just return it as is.
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.
If we will have only one way of querying the API, we should probably use
?url=
instead of sending multiple attributes. The implementation of the extensions is easier (and there are things not possible to implement without it --e.g. intersphinx) and the usage of the endpoint will be simplified too.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.
Using a URL makes it depend on the domain, if the domain changes everything is broken. I'm fine supporting both to support intersphinx.
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.
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'm also not sure if we want to make this a v3 API, given that it works totally differently than the others :/
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.
That's why my note in #8039 (comment)
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 don't think this is super bad. The other endpoints are for resources and this one is not.
However, I'm starting to think that this could be a different service as well? But maybe that's too much? 🤔
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.
Yes and no. Maybe (?), I guess. If they use
https://readthedocs.org/api/v3/embed/?url=
endpoint and it shouldn't break if they don't remove the Domain for that Project from our platform. Even if the DNS for that domain is dead.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 like the idea of listing all the sections. I'd also like to see a way to explore/surf the API just by clicking these links, similar as we do with the
_links
field in other endpoints. This way, you can discover the API by clicking on those, but also expose these links to developers.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.
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'm not sure about the naming conventions here. We are naming a resource, so it should be plural, but we only have a single resource to get the data from.
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.
How would a user build something like this without the ability to also get all the pages in a project? We aren't exposing an API for that in this design doc. Should we?
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.
Hmm, good point, we could have an endpoint for that, but now that you mentioned we need to support external sites, I don't think it would be possible. Or it would be rtd only feature.
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 think so. IIRC @agjohnson idea here was to include the popup/modal form of this into the JS client. Besides, we could eventually be able to use this JS client from our extension itself, instead of maintaining the front-end in our own extension (or adding it as another popup extension together with the ones we currently support)
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.
Yea, the JS client is going to be doing a lot of work in addition to just doing the GET. We definitely want nice default widgets, along with helper methods that make this a much nicer UX for users.