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

Detect embedded and skip addons #415

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

humitos
Copy link
Member

@humitos humitos commented Oct 29, 2024

Check if addons library is being loaded inside an iframe, and skip loading it.

Closes #412

Check if addons library is being loaded inside an iframe, and skip loading it.

Closes #412
@humitos humitos requested a review from a team as a code owner October 29, 2024 15:22
src/utils.js Outdated Show resolved Hide resolved
Copy link
Member

@stsewd stsewd left a comment

Choose a reason for hiding this comment

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

I think this should be an option. We have at least one customer on .com that makes use of iframes, where it does need to show the flyout.

@humitos
Copy link
Member Author

humitos commented Oct 29, 2024

I think this should be an option.

Hrm, good point. I don't think we should expose this to users, since it seems to be pretty complex/advanced/confusing and probably not useful for 99% of our users. However, I think we should probably have an option internally for this and enable/disable it via support request.

We have at least one customer on .com that makes use of iframes, where it does need to show the flyout.

Do they have public docs where I can see how they are using iframes?

@stsewd
Copy link
Member

stsewd commented Oct 29, 2024

Do they have public docs where I can see how they are using iframes?

If I remember correctly, their use case was to load their private documentation using a sharing token from an iframe, probably from inside their application.

However, I think we should probably have an option internally for this and enable/disable it via support request.

We can also expose an option as a query parameter, since the parent page should be the one that decides if the flyout should be shown. This is since users can embed docs they don't have control over. I imagine something like ?rtd-disable-flyout, users embedding docs and that don't want to show the flyout there can include the page with that query.

@humitos
Copy link
Member Author

humitos commented Oct 29, 2024

This is since users can embed docs they don't have control over.

I'm not too worried about this case.

I imagine something like ?rtd-disable-flyout, users embedding docs and that don't want to show the flyout there can include the page with that query.

I think we want to depend on the user for this. How this approach will solve the current problem we have? The user will need to rebuild all their documentation and probably modify the Sphinx extension's code to add that query attribute --which seems pretty complex for regular users.

I think the general rule we were using makes sense, since it follows what we used to have:

  • if it's embed, don't enable addons
  • if it's embed and there is a config that force addons, enable adons
  • if it's not embed, always enable addons

That "config that forces addons" will be a META tag as we are doing with other things we want to communicate from the backend, and it will be a field in AddonsConfig so it can be manged per project. It won't be exposed to users and managed via support request.

This is required for at least one .com customer.
@stsewd
Copy link
Member

stsewd commented Oct 29, 2024

I think we want to depend on the user for this. How this approach will solve the current problem we have? The user will need to rebuild all their documentation and probably modify the Sphinx extension's code to add that query attribute --which seems pretty complex for regular users.

They just need to update the target URL, that's not complex, they are already providing a URL. And that doesn't require users to ask us to disable the option on docs they don't own.

@humitos
Copy link
Member Author

humitos commented Oct 29, 2024

I think we want to depend on the user for this.

I missed typed this. I meant here I think we don't want to depend on the user for this.

@humitos
Copy link
Member Author

humitos commented Nov 5, 2024

I will add an option in the server that by default skips loading the addons in iframes, but that we can manually force injecting addons for those users requiring this feature.

humitos added a commit to readthedocs/readthedocs.org that referenced this pull request Nov 13, 2024
Add an extra HTTP header to decide whether or not force the injection of addons
when the page is embedded (eg. iframe). By default, we are not loading addons if
embedded.

Required by readthedocs/addons#415
Closes readthedocs/addons#412
humitos added a commit to readthedocs/readthedocs.org that referenced this pull request Nov 13, 2024
Add an extra addons config to decide whether or not force the injection of
addons when the page is embedded (eg. iframe). By default, we are not loading
addons if embedded.

Required by readthedocs/addons#415
Closes readthedocs/addons#412
@humitos
Copy link
Member Author

humitos commented Nov 13, 2024

I changed the approach here to use addons.configs.load_when_embedded field from the API. I realized there is no need to use HTML meta tags for this particular case and it's easier, simpler and better to use an API field instead.

humitos added a commit to readthedocs/readthedocs.org that referenced this pull request Nov 13, 2024
Add an extra addons config to decide whether or not force the injection of
addons when the page is embedded (eg. iframe). By default, we are not loading
addons if embedded.

Required by readthedocs/addons#415
Closes readthedocs/addons#412
src/index.js Outdated Show resolved Hide resolved
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.

Duplication of fly-out and banners
3 participants