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

Fix broken flyout layout due to RTD addons upgrade #15485

Merged
merged 3 commits into from
Oct 7, 2024
Merged

Conversation

r0qs
Copy link
Member

@r0qs r0qs commented Oct 7, 2024

RTD is no longer injecting certain HTML elements, such as the flyout menu, and they are replacing these components with addons (see: readthedocs/readthedocs.org#11474). Unfortunately, addons does not work out-of-the-box with the customizations we have implemented - specifically, having a fixed flyout menu at the bottom of the sidebar in our docs.

The issue is because we explicitly use certain CSS selectors (e.g., .rst-versions) to determine where to place the flyout menu, and the current code relied on such selector and didn't properly handle cases where such selector would not be present, breaking the layout with a type error since rstVersions is null. And the same applies for other selectors that are removed in this PR.

rstVersions.remove();

The addons are enabled by default in the RTD dashboard, and it appears that we cannot disable them, see: readthedocs/readthedocs.org#11474 (comment).

You can see the docs of this PR here: https://docs.soliditylang.org/en/rtd-docs-workaround/
And some old broken version here: https://docs.soliditylang.org/en/v0.8.27/ (check the browser console for the error)

Note that this PR does not resolve the layout issues for older versions; it only applies to new releases. To keep older versions somewhat usable, the Flyout option should be enabled in the RTD dashboard.

The PR also introduces checks to safely access properties or call methods on potentially null or undefined objects.

@r0qs r0qs force-pushed the rtd-docs-workaround branch from 2e99129 to 4e58d6c Compare October 7, 2024 12:20
@r0qs r0qs marked this pull request as ready for review October 7, 2024 12:31
ekpyron
ekpyron previously approved these changes Oct 7, 2024
Copy link
Member

@ekpyron ekpyron left a comment

Choose a reason for hiding this comment

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

Assuming that we'd notice it the input[name=mode] things weren't gone in some cases, this is probably safe to merge

@r0qs r0qs force-pushed the rtd-docs-workaround branch from 6ebee1d to 293bd7f Compare October 7, 2024 14:00
ekpyron
ekpyron previously approved these changes Oct 7, 2024
@ekpyron ekpyron self-requested a review October 7, 2024 14:25
@ekpyron ekpyron dismissed their stale review October 7, 2024 14:26

Rodrigo still wanted to look into some other stuff, so I'm dismissing the approval for now.

@r0qs r0qs marked this pull request as draft October 7, 2024 14:28
@r0qs r0qs marked this pull request as ready for review October 7, 2024 15:55
ekpyron
ekpyron previously approved these changes Oct 7, 2024
@r0qs r0qs enabled auto-merge October 7, 2024 16:10
@r0qs r0qs disabled auto-merge October 7, 2024 16:15
@r0qs r0qs force-pushed the rtd-docs-workaround branch 2 times, most recently from 0c42e98 to 847f9ee Compare October 7, 2024 16:49
ekpyron
ekpyron previously approved these changes Oct 7, 2024
Copy link
Member

@ekpyron ekpyron left a comment

Choose a reason for hiding this comment

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

I guess the regexes could be combined into one or two in principle, but also fine with me for now

@ekpyron
Copy link
Member

ekpyron commented Oct 7, 2024

The docs ci build failing is an issue, though :-)

ekpyron
ekpyron previously approved these changes Oct 7, 2024
@r0qs r0qs force-pushed the rtd-docs-workaround branch from ac5c14f to 8d4dc15 Compare October 7, 2024 18:06
Copy link
Member

@ekpyron ekpyron left a comment

Choose a reason for hiding this comment

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

For half of this I'm not 100% clear on why it becomes necessary, but this seems to improve the situation at least, so we can merge and take it from there.

@r0qs r0qs merged commit 0826887 into develop Oct 7, 2024
71 of 72 checks passed
@r0qs r0qs deleted the rtd-docs-workaround branch October 7, 2024 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants