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

Embed APIv3: use latest embed API version #146

Merged
merged 21 commits into from
Sep 28, 2021
Merged

Conversation

humitos
Copy link
Member

@humitos humitos commented Sep 22, 2021

This PR makes usage of the Embed APIv3 that's going to be available in Read the Docs soon. It mainly changes how the API URL is generated. Instead of reading arguments (data-*) injected at build time by Python, it uses only one url= argument which is retrieved from the a.href property via Javascript on hover event.

Note that this reduces the complexity of the Python code a lot since we don't need to resolve references and hard code them into data-* attributes anymore.

ToDo

  • find a way to send the full URL to EmbedAPI (from Python code we can't know the hosted URL)
  • write tests
  • remove all code for other data- elements?
  • remove hardcoded URL
  • remove hardcoded backend api host pointing to ngrok
  • remove restriction about "must be hosted on Read the Docs" to work from the intersphinx docs
  • update documentation
  • remove hoverxref_project and _version to be required
  • QA making sure that all tooltips/links work

Related readthedocs/readthedocs.org#8521

@humitos humitos force-pushed the humitos/embed-api-v3 branch from fb63f85 to e463808 Compare September 22, 2021 13:50
`data['content']` returned by the API is not a single string (not a list) --so,
we need to tell tooltipster to show it as HTML

https://www.heteroclito.fr/modules/tooltipster/#options
The content returned changed from being a list of one element to just a string.
@humitos humitos force-pushed the humitos/embed-api-v3 branch from 63bd0d8 to c8d8016 Compare September 23, 2021 10:23
@humitos humitos force-pushed the humitos/embed-api-v3 branch from c8d8016 to 171920a Compare September 23, 2021 10:27
hoverxref/domains.py Outdated Show resolved Hide resolved
@humitos humitos force-pushed the humitos/embed-api-v3 branch from 5a1efe2 to af03ce8 Compare September 23, 2021 17:02
Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

Looks like a good approach, and much simpler 👍

Might be worth doing a call with @readthedocs/frontend folks to see if there's anywhere to collaborate with the UX while doing some release testing.

docs/conf.py Outdated Show resolved Hide resolved
hoverxref/domains.py Outdated Show resolved Hide resolved
We are not using `data-` attributes anymore. Everything is based on `class=` and
`href=` attributes from the elements.

`hoverxref_project` and `hoverxref_version` are not required anymore and they
are deleted as well.
@humitos humitos marked this pull request as ready for review September 27, 2021 08:59
@humitos humitos requested review from ericholscher and a team September 27, 2021 09:01
Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

This looks like a way nicer way to integrate things. Really glad to see this simplified.

Do we have a release plan for this code documented anywhere? I guess we can install it via git directly to test if we want, but doing some kind of beta release to pypi might be interesting.

hoverxref_api_host = 'https://readthedocs.ngrok.io'
if os.environ.get('NGROK_READTHEDOCS') == 'True':
# Building on a local Read the Docs instance using NGROK for HTTPS
hoverxref_api_host = 'https://readthedocs.ngrok.io'
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this value get pulled from the environment, so other people can use it if needed in dev? eg NGROK_HOVERXREF= https://foo.ngrok.io.

Copy link
Member Author

Choose a reason for hiding this comment

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

Haha! Yes! I don't know why I didn't do it like you are suggesting originally 🙃 It should probably just use hoverxref_api_host all in uppercase.

Copy link
Member Author

Choose a reason for hiding this comment

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

I suppose that I hardcoded it here so I don't have to remember the different URL options 😄


Default: It defaults to ``READTHEDOCS_VERSION`` environment variable
Default: ``https://readthedocs.org``
Copy link
Member

Choose a reason for hiding this comment

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

How are we planning to support this on .org & .com? Should we do this automagically, or just always default to .org?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hrm... good question!

I haven't thought about this, honestly. I don't think we have a way to differentiate if we are running in .org or .com from the builder. It may be good to expose an environment variable? In that case, we can use it to differentiate.

Actually... I just remembered that @stsewd already mentioned this in #134 --so, we should probably continue the conversation there, finish that PR, and merge it. I think it was good, but there was an issue with CORS that I don't remember where we ended up.

tests/test_internals.py Show resolved Hide resolved
@humitos
Copy link
Member Author

humitos commented Sep 27, 2021

Do we have a release plan for this code documented anywhere? I guess we can install it via git directly to test if we want, but doing some kind of beta release to pypi might be interesting.

I was planning to release a 0.8b1 once this and .org branches get merged. I did a bunch of tests locally and everything worked. So, I'm not expecting too many issues --but we will really know once we do the release.

Once we have that release being used for months, I thought we could start thinking about having a 1.0 version; which I think we are close to regarding features and bugs. I don't see too many issues or requested features still around.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants