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.

Sign up for GitHub

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

Jump to bottom

Addons: make default root CSS selector a shared option #11767

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Addons: make default root CSS selector a shared option #11767

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 3 additions & 12 deletions readthedocs/embed/v3/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
from readthedocs.api.v3.permissions import HasEmbedAPIAccess
from readthedocs.core.utils.extend import SettingsOverrideObject
from readthedocs.embed.utils import clean_references
from readthedocs.projects.models import AddonsConfig
from readthedocs.storage import build_media_storage

log = structlog.get_logger(__name__)
Expand Down Expand Up @@ -137,21 +138,11 @@ def _get_content_by_fragment(self, url, fragment, doctool, doctoolversion):
)

def _find_main_node(self, html):
main_node = html.css_first("[role=main]")
# TODO: find a way to get access to ``project.addons.options_doctool_root_selector``
main_node = html.css_first(AddonsConfig.DEFAULT_ROOT_SELECTOR)
if main_node:
log.debug("Main node found. selector=[role=main]")
return main_node

main_node = html.css_first("main")
if main_node:
log.debug("Main node found. selector=main")
return main_node

first_header = html.body.css_first("h1")
if first_header:
log.debug("Main node found. selector=h1")
return first_header.parent

Comment on lines -150 to -154
Copy link
Member

Choose a reason for hiding this comment

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

Seems this part isn't in the new selector? It's not backwards compat, so we should probably keep this here for now?

def _parse_based_on_doctool(self, page_content, fragment, doctool, doctoolversion):
# pylint: disable=unused-argument disable=too-many-nested-blocks
if not page_content:
Expand Down
19 changes: 4 additions & 15 deletions readthedocs/projects/forms.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -651,19 +651,17 @@ class Meta:
fields = (
"enabled",
"project",
"options_doctool_name",
"options_doctool_root_selector",
"analytics_enabled",
"doc_diff_enabled",
"doc_diff_root_selector",
"flyout_enabled",
"flyout_sorting",
"flyout_sorting_latest_stable_at_beginning",
"flyout_sorting_custom_pattern",
"hotkeys_enabled",
"search_enabled",
"linkpreviews_enabled",
"linkpreviews_root_selector",
"linkpreviews_doctool_name",
"linkpreviews_doctool_version",
"notifications_enabled",
"notifications_show_on_latest",
"notifications_show_on_non_stable",
Expand All @@ -679,17 +677,8 @@ class Meta:
),
"notifications_show_on_latest": _("Show a notification on latest version"),
"linkpreviews_enabled": _("Enabled"),
"linkpreviews_root_selector": _("Root selector"),
"linkpreviews_doctool_name": _("Documentation tool name"),
"linkpreviews_doctool_version": _("Documentation tool version"),
}
widgets = {
"doc_diff_root_selector": forms.TextInput(
attrs={"placeholder": AddonsConfig.DOC_DIFF_DEFAULT_ROOT_SELECTOR}
),
"linkpreviews_root_selector": forms.TextInput(
attrs={"placeholder": AddonsConfig.LINKPREVIEWS_DEFAULT_ROOT_SELECTOR}
),
"options_doctool_name": _("Documentation tool"),
"options_doctool_root_selector": _("CSS root selector"),
}

def __init__(self, *args, **kwargs):
Expand Down
67 changes: 67 additions & 0 deletions readthedocs/projects/migrations/0133_addons_standard_root_selector.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
# Generated by Django 4.2.16 on 2024-11-14 13:16

from django.db import migrations, models
from django_safemigrate import Safe


class Migration(migrations.Migration):
safe = Safe.before_deploy

dependencies = [
('projects', '0132_addons_linkpreviews_fields'),
]

operations = [
migrations.RemoveField(
model_name='addonsconfig',
name='doc_diff_root_selector',
),
migrations.RemoveField(
model_name='addonsconfig',
name='linkpreviews_doctool_name',
),
migrations.RemoveField(
model_name='addonsconfig',
name='linkpreviews_doctool_version',
),
migrations.RemoveField(
model_name='addonsconfig',
name='linkpreviews_root_selector',
),
migrations.RemoveField(
model_name='historicaladdonsconfig',
name='doc_diff_root_selector',
),
migrations.RemoveField(
model_name='historicaladdonsconfig',
name='linkpreviews_doctool_name',
),
migrations.RemoveField(
model_name='historicaladdonsconfig',
name='linkpreviews_doctool_version',
),
migrations.RemoveField(
model_name='historicaladdonsconfig',
name='linkpreviews_root_selector',
),
migrations.AddField(
model_name='addonsconfig',
name='options_doctool_name',
field=models.CharField(blank=True, choices=[('sphinx', 'Sphinx'), ('other', 'Other')], help_text='Name of the documentation tool used by this project', max_length=128, null=True),
),
migrations.AddField(
model_name='addonsconfig',
name='options_doctool_root_selector',
field=models.CharField(blank=True, help_text='CSS selector for the main content of the page', max_length=128, null=True),
),
migrations.AddField(
model_name='historicaladdonsconfig',
name='options_doctool_name',
field=models.CharField(blank=True, choices=[('sphinx', 'Sphinx'), ('other', 'Other')], help_text='Name of the documentation tool used by this project', max_length=128, null=True),
),
migrations.AddField(
model_name='historicaladdonsconfig',
name='options_doctool_root_selector',
field=models.CharField(blank=True, help_text='CSS selector for the main content of the page', max_length=128, null=True),
),
Copy link
Contributor

Choose a reason for hiding this comment

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

Just noting that these are add/remove operations, and feel like they could be alters instead. It's probably not necessary, the docdiff selector is probably the only one potentially used by users so far. Maybe a migration is necessary for this at least?

]
39 changes: 18 additions & 21 deletions readthedocs/projects/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,10 +144,11 @@ class AddonsConfig(TimeStampedModel):
Everything is enabled by default.
"""

DOC_DIFF_DEFAULT_ROOT_SELECTOR = "[role=main]"
LINKPREVIEWS_DEFAULT_ROOT_SELECTOR = "[role=main] a.internal"
LINKPREVIEWS_DOCTOOL_NAME_CHOICES = (
DEFAULT_ROOT_SELECTOR = ":is([role=main],main,div.body,div.document)"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is is() required? querySelectorAll("[role=main], main, div.body, div.document") should be the same. Does css_first support this?

DOCTOOL_NAME_CHOICES = (
Copy link
Member

Choose a reason for hiding this comment

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

Seems like we're going to want more of these over time, and some way to have users input one, but we can start with this for now I guess.

("sphinx", "Sphinx"),
("mkdocs", "MkDocs"),
("docusaurus", "Docusaurus"),
("other", "Other"),
)

Expand All @@ -167,6 +168,20 @@ class AddonsConfig(TimeStampedModel):
help_text="Enable/Disable all the addons on this project",
)

options_doctool_name = models.CharField(
choices=DOCTOOL_NAME_CHOICES,
null=True,
blank=True,
max_length=128,
help_text="Name of the documentation tool used by this project",
)
options_doctool_root_selector = models.CharField(
Copy link
Member

Choose a reason for hiding this comment

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

This is a UX issue, but it feels weird to expose these at the top level of Addons. I think most users don't have any idea what to do with this, especially since the UI has help_text hidden, and there's no HTML placeholder. If we're going to be surfacing this so prominently, we definitely need more UI to guide users about what to put here, but I'm pretty skeptical that most users will need to adjust this, and will know when and how to do it?

null=True,
blank=True,
max_length=128,
help_text="CSS selector for the main content of the page",
)

# Analytics

# NOTE: we keep analytics disabled by default to save resources.
Expand All @@ -177,12 +192,6 @@ class AddonsConfig(TimeStampedModel):
doc_diff_enabled = models.BooleanField(default=True)
doc_diff_show_additions = models.BooleanField(default=True)
doc_diff_show_deletions = models.BooleanField(default=True)
doc_diff_root_selector = models.CharField(
null=True,
blank=True,
max_length=128,
help_text="CSS selector for the main content of the page",
)

# EthicalAds
ethicalads_enabled = models.BooleanField(default=True)
Expand Down Expand Up @@ -226,18 +235,6 @@ class AddonsConfig(TimeStampedModel):

# Link Previews
linkpreviews_enabled = models.BooleanField(default=False)
linkpreviews_root_selector = models.CharField(null=True, blank=True, max_length=128)
linkpreviews_doctool_name = models.CharField(
choices=LINKPREVIEWS_DOCTOOL_NAME_CHOICES,
null=True,
blank=True,
max_length=128,
)
linkpreviews_doctool_version = models.CharField(
null=True,
blank=True,
max_length=128,
)


class AddonSearchFilter(TimeStampedModel):
Expand Down
5 changes: 3 additions & 2 deletions readthedocs/proxito/views/hosting.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -513,7 +513,7 @@ def _v1(self, project, version, build, filename, url, request):
"linkpreviews": {
"enabled": project.addons.linkpreviews_enabled,
"root_selector": project.addons.linkpreviews_root_selector
or project.addons.LINKPREVIEWS_DEFAULT_ROOT_SELECTOR,
or project.addons.DEFAULT_ROOT_SELECTOR,
"doctool": {
"name": project.addons.linkpreviews_doctool_name,
"version": project.addons.linkpreviews_doctool_version,
Expand Down Expand Up @@ -587,7 +587,8 @@ def _v1(self, project, version, build, filename, url, request):
)
if filename
else None,
"root_selector": project.addons.doc_diff_root_selector,
"root_selector": project.addons.doc_diff_root_selector
or AddonsConfig.DEFAULT_ROOT_SELECTOR,
"inject_styles": True,
# NOTE: `base_host` and `base_page` are not required, since
# we are constructing the `base_url` in the backend instead
Expand Down
17 changes: 6 additions & 11 deletions readthedocs/search/parsers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,21 +111,16 @@ def _get_main_node(self, html):
so they are children of the same parent node.
- Return the body element itself if all checks above fail.
"""
body = html.body
main_node = body.css_first("[role=main]")
if main_node:
return main_node
Comment on lines -114 to -117
Copy link
Member

Choose a reason for hiding this comment

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

I know that we had role=main for old versions of sphinx, they may see some extra content indexed now.

from readthedocs.projects.models import AddonsConfig

main_node = body.css_first("main")
body = html.body
main_node = body.css_first(
self.project.addons.options_doctool_root_selector
or AddonsConfig.DEFAULT_ROOT_SELECTOR
)
if main_node:
return main_node

# TODO: this could be done in smarter way,
# checking for common parents between all h nodes.
first_header = body.css_first("h1")
Copy link
Member

Choose a reason for hiding this comment

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

Another place we're losing this h1 logic as a fallback. Seems important, since otherwise we're changing how it works again.

if first_header:
return self._get_header_container(first_header).parent

return body

def _get_header_container(self, h_tag):
Expand Down