-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Flyout and Footer API design document #8052
Conversation
.. code-block:: javascript | ||
|
||
window.readthedocs = window.readthedocs || {}; | ||
window.readthedocs.customizations = {disable_custom_search: true, disable_version_selector: true}; |
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 already do this, but we haven't document them. I like more the name you are proposing here p:
READTHEDOCS_DATA.features.docsearch_disabled
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 know we do this. I like the idea a lot but I wish it was easier for users to control as well as documented. The way it is now can run into race conditions with us defining these features and users setting them.
.. code-block:: javascript | ||
|
||
window.readthedocs = window.readthedocs || {}; | ||
window.readthedocs.integration_version_selector = function () {}; |
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'd name this custom_version_selector maybe? Wasn't obvious for me that this was a custom function to inject the footer.
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'd also call it something other than version_selector
since it does a lot more than that.
|
||
|
||
Overlap with existing API endpoints | ||
----------------------------------- |
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.
Just a note here that we are using the proxied endpoint of the footer, this /_/api/v2/footer_html
, this allow us to share the same domain with some advantages:
- avoid being blocked by some extensions
- Allow to cache the response (docs are served from cloudflare)
- Allow to use the auth backends in .com
And, I've been having some ideas around our APIs... here is an extract from #8039
We should probably make a distinction between our general API that handles Read the Docs resources,
vs our APIs that expose features (like server side search, footer, and embed, all of them proxied).
This way we can version each endpoint separately.
I do agree that the footer shares a lot with the projects/ endpoint. Proxying api v3 may be a little of a problem, because it relies heavily on resolving URLs from the dashboard, but in proxito we don't have the url patterns from the main site.
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.
This is a very good point. Maybe we don't need all the APIv3 endpoints but we can have an endpoint that just returns the same data as the APIv3 endpoint and shares code.
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 proposal looks good to me. I don't have too many comments on the JS implementation side, but seems it will work to me.
I'm not convinced that it's a good idea to share the APIv3 endpoint for the flyout to a regular endpoint. I think having independence and granular control here is better.
Currently, there are separate API endpoints for translations and downloads, | ||
but ideally all the data needed to generate a version selector would be available from a single API. |
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.
It seems that we could implement ?expand=translations,subrpojects
and we would have everything we need with one API call: https://readthedocs.org/api/v3/projects/docs/?expand=active_versions,translations,subprojects
One downside that has using this endpoint is that we will be returning much data than we need and that could slow down the response time.
Another thing to consider is that APIv3 is authenticated-only, and we require anonymous access for the flyout.
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 sold on the idea of sharing the endpoint for the flyout than a regular endpoint. In the future we may want to add extra data to /api/v3/projects/
and we may be blocked because it increases the response time by some milliseconds that are not acceptable for the flyout endpoint.
I think that having an independent endpoint with more control over it is a win here and will avoid headaches in the future.
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.
Agreed here. I think we want a dedicated API just for this use, but it could share code for sure. The footer API is large enough of a use-case to make it special.
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.
Also agree here, one flyout API seems like the best method. We don't yet have a good pattern for injecting an API access token into the build process, and this would be needed for all of the authenticated API endpoints.
Co-authored-by: Manuel Kaufmann <[email protected]>
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.
This is a good start. I think we probably need to clarify our names a good bit here, but I like the general approach here. I don't know enough about JS to really judge alternative implementations though. I know our ad client works in a much more complicated way, but I'm not sure if that's because it's used on more sites, or why that design was chosen.
In particular, I think we need to clarify and standardize on names for:
- What this API is called.
footer
isn't really correct. I think probably it'sflyout_data
or something like that?dynamic
project_data`? - What the flyout is called? I think we generally standardized on
flyout
but this doc usesversion_selector
. Should we removeflyout
for the various composite parts?
I'm sure there's more named needed, but those were the two obvious ones. Names are hard.
.. code-block:: javascript | ||
|
||
window.readthedocs = window.readthedocs || {}; | ||
window.readthedocs.integration_version_selector = function () {}; |
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'd also call it something other than version_selector
since it does a lot more than that.
Currently, there are separate API endpoints for translations and downloads, | ||
but ideally all the data needed to generate a version selector would be available from a single API. |
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.
Agreed here. I think we want a dedicated API just for this use, but it could share code for sure. The footer API is large enough of a use-case to make it special.
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 still considering the effects of using global state to configure the client. Generally, having a fully instantiated object is a more expected pattern and an easier pattern to work with if the user wants to extend functionality instead of replace it.
|
||
// This first line helps handle the case where the project/theme's overrides | ||
// are executed before the readthedocs global is defined. | ||
window.readthedocs = window.readthedocs || {}; |
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.
One seemingly common use case that is not supported by this current proposal is extending the flyout display function/method. If a theme wants the flyout, but also wants to inject a version selector somewhere else in the UI, there is no way to extend this functionality, only override it.
That is, the user is expected to define a window.readthedocs.integration_version_selector
to customize the display process, but there is no way to call the parent window.readthedocs.integration_version_selector
function -- it isn't defined until after it is inspected by our client.
This could either be a reason to use a different pattern than defining overrides as global state, or we would need to require more granular override points for users.
Currently, there are separate API endpoints for translations and downloads, | ||
but ideally all the data needed to generate a version selector would be available from a single API. |
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.
Also agree here, one flyout API seems like the best method. We don't yet have a good pattern for injecting an API access token into the build process, and this would be needed for all of the authenticated API endpoints.
* This proposal doesn't involve creating an integration point to control custom search. | ||
That could happen at a later date. | ||
* This proposal doesn't rework how the version selector looks either on the RTD Sphinx theme | ||
or on other themes by default. Any restyling can be done independently of this. |
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.
Also, for this proposal to work for commercial users that are currently manipulating the flyout via CSS rules, we have to output the exact same DOM structure that we would normally return with the HTML blob. This is a little unfortunate, as the DOM structure could already stand to be designed better. We can adapt this later, but this might also hint at some support for multiple versions of DOM output.
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. |
Things have changed considerably here with the introduction of the addons in https://github.com/readthedocs/readthedocs-client. I think it's safe to close this PR or, if we want, merge and close so we don't loose the document. Maybe adding a note in the top of it linking to the addons repository? |
Maybe we just keep this available for reference on this branch? Merging this could be confusing for anyone that comes across this first, as we deviated from this plan a bit. I agree adding a note would help either way though 👍 |
I updated the warning note and I will merge it after that. |
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 approving this PR so it gets merged automatically after the check pass. Note that this work is not implemented and most of it was replaced by https://github.com/readthedocs/readthedocs-client and its new endpoint (/_/readthedocs-config/
)
This is a design document for using APIv3 in order to generate the flyout (version selector) menu. This is purely a proposal at this point to get everyone on the same page with respect to goals and how it could be implemented.
The main goals are to:
For easier reading, there's a link to the document.
Closes #7748