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

Integrate with error pages #371

Closed
agjohnson opened this issue Sep 9, 2024 · 3 comments · Fixed by #373
Closed

Integrate with error pages #371

agjohnson opened this issue Sep 9, 2024 · 3 comments · Fixed by #373
Assignees
Labels
Accepted Accepted issue on our roadmap Improvement Minor improvement to code

Comments

@agjohnson
Copy link
Contributor

The documentation error pages used to direct users to search in our dashboard with an input. I noted here that we should probably rework the search feature to instead use the in-doc search instead:

One other point to integrating with the error pages is that Addons are active on error pages currently -- flyout, version warnings, etc. These feel a little out of place on error pages, and might be conflicting warnings to readers.

@agjohnson agjohnson added Improvement Minor improvement to code Needed: design decision A core team decision is required labels Sep 9, 2024
@agjohnson
Copy link
Contributor Author

We should disable the flyout and notifications on error pages probably, otherwise they both show by default:

image

We want analytics addon to always run though, and might use search in these UIs eventually. So we shouldn't disable all addons on error pages.

@github-project-automation github-project-automation bot moved this to Planned in 📍Roadmap Sep 17, 2024
@agjohnson agjohnson added the Accepted Accepted issue on our roadmap label Sep 17, 2024
@agjohnson agjohnson added this to the Dashboard minor bugs milestone Sep 17, 2024
@humitos
Copy link
Member

humitos commented Sep 18, 2024

I noted here that we should probably rework the search feature to instead use the in-doc search instead:

👍🏼

We should disable the flyout and notifications on error pages probably, otherwise they both show by default
We want analytics addon to always run though, and might use search in these UIs eventually. So we shouldn't disable all addons on error pages.

URL where you can notice this behavior: https://dev.readthedocs.io/en/latest/notfound.html

I've tried this some time ago and I found that's not possible to know what's the status code for the original documentation URL from JavaScript. I will add the status code using CF Worker and reading it from JavaScript following the pattern we are using to read other similar data we don't have access from JavaScript.

humitos added a commit to readthedocs/common that referenced this issue Sep 18, 2024
We need to know what's the HTTP status for the original request to be able to
decide what addons will be enabled. There are some addons we want to disable on
404s for example.

Related: readthedocs/addons#371
humitos added a commit that referenced this issue Sep 18, 2024
It reads a META tag `readthedocs-http-status` to get the HTTP status code for
the original request to the documentation URL.

It uses this HTTP status code to decide if each of the addons should be enabled
or not.

Closes #371
Requires readthedocs/common#222
@humitos
Copy link
Member

humitos commented Sep 18, 2024

I opened these PRs to solve this issue:

@humitos humitos self-assigned this Sep 18, 2024
@humitos humitos removed the Needed: design decision A core team decision is required label Sep 18, 2024
@humitos humitos moved this from Planned to Needs review in 📍Roadmap Sep 18, 2024
humitos added a commit to readthedocs/common that referenced this issue Sep 19, 2024
We need to know what's the HTTP status for the original request to be able to
decide what addons will be enabled. There are some addons we want to disable on
404s for example.

Related: readthedocs/addons#371
@github-project-automation github-project-automation bot moved this from Needs review to Done in 📍Roadmap Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted Accepted issue on our roadmap Improvement Minor improvement to code
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants