Addons: make default root CSS selector a shared option #11767
Conversation
Add a "global" shared CSS root selector that is used all across our features: link previews, docdiff, search, embed API, etc. Also, put the "documentation tool name" as a top level config as well. With that information we can define better defaults for CSS selectors and other stuffs in the future if needed. Example: if documentation tool is set as Sphinx by the user, we will be able to use `[role=main] a.internal` for the Link Preview CSS selector.
humitos
force-pushed
the
humitos/addons-css-selector
branch
from
1a280a3
to
44587ab
Compare
humitos
changed the title
humitos
requested review from
ericholscher and
agjohnson
and removed request for
stsewd
| 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)" |
There was a problem hiding this comment.
Is is() required? querySelectorAll("[role=main], main, div.body, div.document") should be the same. Does css_first support this?
| 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), | ||
| ), |
There was a problem hiding this comment.
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?
| first_header = html.body.css_first("h1") | ||
| if first_header: | ||
| log.debug("Main node found. selector=h1") | ||
| return first_header.parent | ||
|
|
There was a problem hiding this comment.
Seems this part isn't in the new selector? It's not backwards compat, so we should probably keep this here for now?
| LINKPREVIEWS_DEFAULT_ROOT_SELECTOR = "[role=main] a.internal" | ||
| LINKPREVIEWS_DOCTOOL_NAME_CHOICES = ( | ||
| DEFAULT_ROOT_SELECTOR = ":is([role=main],main,div.body,div.document)" | ||
| DOCTOOL_NAME_CHOICES = ( |
There was a problem hiding this comment.
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.
| 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") |
There was a problem hiding this comment.
Another place we're losing this h1 logic as a fallback. Seems important, since otherwise we're changing how it works again.
| max_length=128, | ||
| help_text="Name of the documentation tool used by this project", | ||
| ) | ||
| options_doctool_root_selector = models.CharField( |
There was a problem hiding this comment.
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?
| body = html.body | ||
| main_node = body.css_first("[role=main]") | ||
| if main_node: | ||
| return main_node |
There was a problem hiding this comment.
I know that we had role=main for old versions of sphinx, they may see some extra content indexed now.
Add a "global" shared CSS root selector that is used all across our features: link previews, docdiff, search, embed API, etc.
Also, put the "documentation tool name" as a top level config as well. With that information we can define better defaults for CSS selectors and other stuffs in the future if needed.
Example: if documentation tool is set as Sphinx by the user, we will be able to use
[role=main] a.internalfor the Link Preview CSS selector.Related readthedocs/addons#433