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

Post: enable addons by default #308

Merged
merged 6 commits into from
Jul 15, 2024
Merged

Post: enable addons by default #308

merged 6 commits into from
Jul 15, 2024

Conversation

humitos
Copy link
Member

@humitos humitos commented Jul 8, 2024

@humitos humitos requested a review from ericholscher July 8, 2024 11:26
@humitos humitos requested a review from a team as a code owner July 8, 2024 11:26
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.

I feel like this post needs a tl;dr about the context removal, and we need to be much more explicit around the fact it's happening. Addons being enabled is one thing, but the build changes are pretty big, and we want to highlight the extension, and explain more how it works, I think?

content/posts/addons-by-default.md Outdated Show resolved Hide resolved
content/posts/addons-by-default.md Outdated Show resolved Hide resolved
content/posts/addons-by-default.md Outdated Show resolved Hide resolved
content/posts/addons-by-default.md Outdated Show resolved Hide resolved
content/posts/addons-by-default.md Outdated Show resolved Hide resolved
@humitos
Copy link
Member Author

humitos commented Jul 9, 2024

I agree we can add a warning note, an extra paragraph with some bold text or similar alerting users about the build context changes. However, we don't want all users to go and blindly install the extension. I understand that a good tl;dr would be something like: "Read the Docs stopped adding extra context and manipulating configuration on Sphinx builds. If you are experimenting any issue after addons are enabled by default, read "How does it affect my projects?" section and try installing sphinx-build-compatibility"

On the other hand, I think going too deep technically on what exactly has changed, what are the exact variables that are not passed anymore, how to configure those variables by yourself, etc won't be valuable here and won't help most of the users and I think it will only confuse them more than help. They aren't already aware of those, and for those users needing them, the solution is just to install the Sphinx extension --we don't need to explain this on details to regular users.

@humitos
Copy link
Member Author

humitos commented Jul 9, 2024

I went ahead and added a TL;DR small section at the beginning of the post. Besides, I added a small chunk of Python code that explains how to setup the canonical URL if they are using a custom domain, plus the READTHEDOCS variable in the context.

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 better. I'm fine not highlighting the context removal as much, but I do think it will break more than we think it will, especially with all the themes, etc. 👍

content/posts/addons-by-default.md Outdated Show resolved Hide resolved
@humitos
Copy link
Member Author

humitos commented Jul 10, 2024

Thanks for the review.

I'm happy to come back an update the post with some feedback received once we enabled it for new projects. We can also write a "Migration guide" in our documentation with more technical details if needed. We can also create an issue in our repository that links to this post and pin it; so users find it immediately when they go to open an new one about a related problem.

I targeted this post for next Monday, but we can merge it before that date if we are already happy with it. Let me know.

@ericholscher
Copy link
Member

Yea, I think making sure we have a few places where we're noting this is important. 👍 I'm fine shipping it on Monday.

@humitos humitos added the Status: blocked Issue is blocked on another issue label Jul 10, 2024
@humitos humitos removed the Status: blocked Issue is blocked on another issue label Jul 15, 2024
@humitos humitos merged commit 92f2109 into main Jul 15, 2024
4 checks passed
@humitos humitos deleted the humitos/addons-by-default branch July 15, 2024 12:13
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.

2 participants