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: Add anchor offset for <dt> anchor links #280

Merged
merged 2 commits into from
Nov 5, 2020

Conversation

hoetmaaiers
Copy link
Collaborator

This fixes #267

@choldgraf
Copy link
Collaborator

I think you might have a merge resolution leftover in your code :-)

image

though I think the fix does work!

https://pydata-sphinx-theme--280.org.readthedocs.build/en/280/demo/api.html#numpy.linalg.matrix_power

@@ -17,6 +17,7 @@

{% macro head_pre_bootstrap() %}
<link href="{{ pathto('_static/css/theme.css', 1) }}" rel="stylesheet" />
<<<<<<< HEAD
Copy link
Collaborator

Choose a reason for hiding this comment

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

ah yeah - looks like you need to resolve this merge

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah darn, thought the yarn build solved this. Cleared the merge conflicts, ran yarn build again...

Copy link
Member

Choose a reason for hiding this comment

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

I have learned to always ignore merge conflicts in this auto-generated file, but just let the build regereate it ..

@jorisvandenbossche
Copy link
Member

@hoetmaaiers thanks for looking into this!

Checking the demo docs, the link is now working correctly, but it seems to have some undesired side effects on the rest of the layout ..

This PR: https://pydata-sphinx-theme--280.org.readthedocs.build/en/280/demo/generated/pandas.DataFrame.drop.html#pandas.DataFrame.drop

image

Master: https://pydata-sphinx-theme.readthedocs.io/en/latest/demo/generated/pandas.DataFrame.drop.html#pandas.DataFrame.drop

image

@jorisvandenbossche
Copy link
Member

It seems that the background-color: white; is the problem, but when removing that, the yellow box gets expanded ..

@hoetmaaiers
Copy link
Collaborator Author

hoetmaaiers commented Nov 4, 2020

This is indeed a strong issue. I assume we want to keep the highlighted box since this adds some good user experience.
Would it be possible to extend the <dt> element output?

By wrapping the dt content in an extra div (e.g. dt-content, I think the problem could be addressed:

<dt id="numpy.linalg.matrix_power">
    <div class="dt-content">
        <code class="sig-prename descclassname">numpy.linalg.</code>
        <code class="sig-name descname">matrix_power</code>
        <span class="sig-paren">(</span>
        <em class="sig-param">
            <span class="n">a</span>
        </em>,
        <em class="sig-param">
            <span class="n">n</span>
        </em>
        <span class="sig-paren">)</span>
        <a class="headerlink" href="#numpy.linalg.matrix_power" title="Permalink to this definition"></a>
    </div>
</dt>

@jorisvandenbossche
Copy link
Member

This is indeed a strong issue. I assume we want to keep the highlighted box since this adds some good user experience.

Hmm, actually I am not sure it is that important. At least for doc sites like pandas, where each docstring gets its own page, I don't think it adds much value. But indeed, for pages with many docstrings under each other on a single page, highlighting the linked one can be useful.

Would it be possible to extend the

element output?

Unfortunately, I don't think this is that easy to modify the generated html (I would need to look into sphinx' autodoc extension and the directive it uses). We might be able to add a class if we override that similarly as we do for table: https://github.com/pandas-dev/pydata-sphinx-theme/blob/edc2a0bdee1411ac9f7d5950fcacb4720a7134e0/pydata_sphinx_theme/bootstrap_html_translator.py#L23, but would need to look into it to know.

@hoetmaaiers
Copy link
Collaborator Author

After some more thinking (and after a good nights rest), I solved the problem by being more specific in the css selector for

. The undesired side effects should be solved with this.

@jorisvandenbossche jorisvandenbossche changed the title Add anchor offset for <dt> anchor links FIX: Add anchor offset for <dt> anchor links Nov 5, 2020
@jorisvandenbossche jorisvandenbossche merged commit e987558 into pydata:master Nov 5, 2020
@jorisvandenbossche
Copy link
Member

Cool, that's an easier solution, thanks!

One other question, but can be as a follow-up if needed: now we have two places in the scss code that defines very similar styling for a &::before element. Would it be possible to somehow combine those two? With a combined / generic selector (or maybe using :target, cfr https://stackoverflow.com/a/28824157/653364 / https://developer.mozilla.org/en-US/docs/Web/CSS/:target).
That might also solve anchors that are not atached to a specific header or definition list or table, see my comment about that on the issue: #267 (comment)

@hoetmaaiers
Copy link
Collaborator Author

Cool, now I learned about :target. This could indeed be an even better and more generic solution!

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.

anchor links hidden by header navbar
3 participants