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 extra whitespace in sidebars #1115

Merged
merged 8 commits into from
Jan 13, 2023
Merged

Conversation

choldgraf
Copy link
Collaborator

@choldgraf choldgraf commented Jan 11, 2023

This refactors some of our sidebar template logic in order to avoid having "empty but still present" sidebar items. It does so with the following steps:

  • Adds a section to our HTML template event that handles removing templates that we know to be empty.
  • Checks for specific configuration for the edit-this-page and show-sourcelink to remove these templates if the user has configuration to do so (even if the template is still part of the template list)
  • Moves our "is the page toc empty?" check to this Python function rather than being part of the jinja HTML template.
  • Fixes a doc that incorrectly said this was related to sphinx.ext.viewcode
  • Moves the searchbox.html out of the secondary sidebar and instead to just above the article content. This way the presence of this button does not depend on their being a secondary sidebar. It also means there is not an "empty whitespace" in the sidebar because of the padding given to all toc-item elements. It will now show just above the page title.
  • This means that on pages without the sourcelink/edit this page, and with no in-page TOC, the secondary sidebar will hide completely by default. closes Empty TOC in right sidebar #854 closes Extra vertical whitespace padding above h1 in theme v0.10.0 #955
  • While I'm at it, add a check for whether globalcontext exists and only use it if so. closes Error in _overwrite_pygments_css: DirectoryHTMLBuilder' object has no attribute 'globalcontext' #1096

We could do something a little more clever, and actually try to render each template and manually check if it is empty. But I didn't do this because I was worried it'd be too computationally expensive. If others think it's not a big deal, I could try that (perhaps skipping our page-toc and site-toc templates). The code would be roughly:

rendered = app.builder.templates.render("TEMPLATENAME", context)
if len(rendered) == 0:
    # remove template

@choldgraf
Copy link
Collaborator Author

hmmm @drammock it seems that latest commit made the tests start to fail 🤔

@drammock
Copy link
Collaborator

hmmm @drammock it seems that latest commit made the tests start to fail thinking

pushed a commit that fixes it (locally at least, but should pass CIs too)

Copy link
Collaborator

@12rambau 12rambau left a comment

Choose a reason for hiding this comment

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

I see the point of moving away the searchbox.html template from the secondary sidebar but I think it will be invisible for user where you display it now.

Use case:
I search the word "Customize logo link" in our doc and click on the first link (branding page). As the result is not on the first line of the page the "hide search result button" is simply invisible as it's on the top of the page.

What if instead we identify the status of the searchbox (displayed or not) and use a css selector to display (or not) the secodary sidebar. This behavior should be only triggered if the searchbox is the only item in it. This can be setup with an extra class to the secondary sidebar such as only-searchbox.

Advantage:

  • make good use of your new python script to guess if it's a "only-searchbox" sidebar
  • the button remain visible wherever you scroll
  • nicer look (as the button was designed to be there in the first place)

what do you think ?

@choldgraf
Copy link
Collaborator Author

choldgraf commented Jan 12, 2023

Two quick updates:

  • I realized that there was another "ghost template" at the bottom of the primary sidebar, so I decided to go ahead and generalize the function to remove any empty template, rather than special-casing each one. This checks every single template, except for a few hard-coded ones that we know take longer to render. So build time shouldn't be affected. I also added a test.
  • I'm not sure what you mean by What if instead we identify the status of the searchbox (displayed or not) and use a css selector to display (or not) the secodary sidebar. Do you mean identify it with JavaScript manually? The challenge with CSS is that you can't define a rule for a parent based on the selector for a child. Another concern I'd have with this is that when somebody clicked the button, it would suddenly make all of the content expand which might be jarring.

Another option is we could make the button floating in the bottom-right - then it's always visible but doesn't break the flow of the page at all...

@12rambau
Copy link
Collaborator

The challenge with CSS is that you can't define a rule for a parent based on the selector for a child.

damned, you're right. Ok then second suggestion: a customized btn (too make it look like a versionadded directive but with info type) and set it fixed on top of the article. As such we don't break the display by addign extra stuff on the right and we get access to the btn anywhere we scroll.

Another option is we could make the button floating in the bottom-right

This is were the RDT fly-out was and phone users were complaining a lot. Not sure setting anything there will make people happy

@choldgraf
Copy link
Collaborator Author

How about we do your suggestion here:

a customized btn (too make it look like a versionadded directive but with info type) and set it fixed on top of the article

but put it both above and below the article, so that a user has a higher chance of seeing it quickly. We might need to modify the JS to handle two buttons rather than one but that doesn't seem too tough.

@choldgraf
Copy link
Collaborator Author

Argh so we can't put it both above and below because the searchbox JavaScript is a sphinx-specific thing, not JS that we define.

So in the latest commit I tried something else: putting it in the primary sidebar at the top, rather than the secondary sidebar. My reasoning is that it is more common for the primary sidebar to be present than the secondary sidebar, and it is also visible anywhere, so this will make it easier to notice and use. This also puts it in the same place as other "control the site behavior" switches like the theme switcher.

Here's how that looks:

chrome_LbaPDg9L1P

Another option is that we could actually make this an icon, and put it next to the search icon. e.g., an X icon that shows up next to the search icon.

@12rambau
Copy link
Collaborator

I like the idea but If you set it in the primary sidebar you will end up with the same problem as in #1092.
I started doing simple documentation recently using our theme and the primary sidebar is often removed (I have like 3 pages and basta): https://pygadm.readthedocs.io/en/stable/usage.html

What is clear is that making both the sidebars optionals is considerably reducing our options for extra information...
header navbar is indeed the only one always present but will people see the extra btn ?

@choldgraf
Copy link
Collaborator Author

Hmmm good point - how about we go with your original proposal but put it at the top of the page. I think that people will naturally scroll around a bit and will notice it there if we make the style noticeable. If people raise complaints that they can't find it, we can find another UX for it.

@choldgraf
Copy link
Collaborator Author

choldgraf commented Jan 13, 2023

OK the latest commit moves the button back to the top of the article, it also makes it a little bit bigger and styles it similar to the sphinx-design buttons on this page: https://pydata-sphinx-theme.readthedocs.io/en/stable/user_guide/web-components.html#badges-and-buttons. I made the button a little bit bigger than normal, so that it is a bit more noticeable. I figure it's not such a big deal because this button is rarely on pages and only after someone has explicitly searched for something. But happy to make it smaller if folks prefer.

Edit: now that I've looked at the GIF looping a few times...I'm thinking the button is too big...let me know if you agree :-)

Here's a GIF of the UX:

chrome_Ee2vnC1xJr

Copy link
Collaborator

@12rambau 12rambau left a comment

Choose a reason for hiding this comment

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

I like the last itteration.

I don't know yet about the btn, I would say let's send it and see what folks have to say and we'll customize it in a follow up PR.

The most urgent thing here is to get rid of the secondary navbar !


Environments are re-used by default. Use the following-pattern to re-install them.

nox -s docs -- -r
Copy link
Collaborator

Choose a reason for hiding this comment

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

❤️ I did not know this shorcut, I've been using --no-reuse-existing-virtualenvs which is so long to type that it hurts.

@drammock drammock merged commit aacaac3 into pydata:main Jan 13, 2023
12rambau added a commit to 12rambau/pydata-sphinx-theme that referenced this pull request Mar 2, 2023
* Fix extra whitespace in sidebars (pydata#1115)

* Fix extra whitespace in sidebars

* Searchbox

* Update src/pydata_sphinx_theme/__init__.py

Co-authored-by: Daniel McCloy <[email protected]>

* make test pass

* Fix template filter to remove empty files

* ABlog in template test

* Move clear search button to primary sidebar

* Move search clear button to top of article

Co-authored-by: Daniel McCloy <[email protected]>

* FIX: Use logo_url instead deprecated logo in theme (pydata#1094) (pydata#1097)

resolves pydata#1094

* ENH/MAINT: avoid overwriting the HtmlTranslator (pydata#1105)

Co-authored-by: Chris Holdgraf <[email protected]>
Fix pydata#143
Fix pydata#94

* fix: align sidebar sliding with the buttons (pydata#1123)

* fix: aline the sidebar sliding with the buttons

* build: force test to run on all platform
if one platform is failing we cannot see if it's platform related as the other were closed

* fix: use correct path for documentation logo

* MAINT: Improve font sizing (pydata#1129)

Fix pydata#1001

* MAINT: Refactor workflows to reduce test dependencies (pydata#1136)

* MAINT: update prerelease workflow (pydata#1140)

* ABlog: Updates for new HTML structure (pydata#1118)

* ABlog: Updates for new HTML structure

* Update templates for latest release

* Bump to dev0

* Standardize logo image behavior between Sphinx and this theme (pydata#1132)

Co-authored-by: Daniel McCloy <[email protected]>
Co-authored-by: Chris Holdgraf <[email protected]>
Co-authored-by: Chris Holdgraf <[email protected]>

* 0.13.0rc1

* Build(deps): Bump http-cache-semantics from 4.1.0 to 4.1.1 (pydata#1154)

* DOC: Use only shield.io for badges in README (pydata#1152)

* Copyright semicolon (pydata#1160)

* FIX: Flex behavior should shrink header items instead of brand (pydata#1158)

Co-authored-by: Chris Holdgraf <[email protected]>
Fixes pydata#1143

* STYLE: lint the documentation with Doc8 (pydata#1150)

Fix pydata#1139

* Add test for internationalization and translations (pydata#1138)

* FIX: Javascript incorrect check for variable (pydata#1166)

* MAINT: update pypi classifiers (pydata#1153)

Fix pydata#1106

* remove emoji from landing page (pydata#1151)

* add fa icons instead of emoji

* remove fix for emojis

* use markup for readme emojis

* use pst-color-primary instead of sd-text-primary

* make our semantic colors available as classes

* try again

---------

Co-authored-by: Daniel McCloy <[email protected]>

* FIX: Narrow scope of style rule for GitHub & GitLab link shortening (pydata#1167)

Fixes pydata#1156

* ENH: Add breadcrumbs to article header (pydata#1142)

* ENH: Add breadcrumbs to article header

* Update src/pydata_sphinx_theme/theme/pydata_sphinx_theme/components/breadcrumbs.html

Co-authored-by: Tania Allard <[email protected]>

* More fixes

* Improving nested page behavior

* Documenting breadcrumbs

* Update src/pydata_sphinx_theme/assets/styles/components/_breadcrumbs.scss

Co-authored-by: Rambaud Pierrick <[email protected]>

* Breacrumbs have link color

---------

Co-authored-by: Tania Allard <[email protected]>
Co-authored-by: Rambaud Pierrick <[email protected]>

* Degrade gracefully when JavaScript is disabled (pydata#1146)

* Fix header vertical spacing and jupyter-sphinx cells (pydata#1164)

Fixes undefined

* RLS: v0.13.0rc2 (pydata#1170)

* DOCS: admonition customization (pydata#1155)

* first draft of the admonition customization

* typo in doc link

* flesh out admon. customization example; DRY

* use :code:rst instead of :literal:

* Update docs/_static/custom.css

---------

Co-authored-by: Daniel McCloy <[email protected]>

* Fix article header CSS (pydata#1171)

* “Edit this page” → “Edit on GitHub/GitLab/Bitbucket” (pydata#1177)

* “Edit this page” → “Edit on GitHub/GitLab/Bitbucket”

Fixes pydata#1172

* Add tests

* Fix typo

* Properly handle default_mode=auto when writing logos (pydata#1183)

We used to only defaulting to the light version when `default_mode` was
undefined, not when it was explicitly set to `auto`. We also need to
handle the latter, as the new test shows.

Closes pydata#1180

Co-authored-by: Jérémy Bobbio (Lunar) <[email protected]>

* fix: correctly add DOM listeners (pydata#1179)

fix adding DOM listeners

* maint: update GitLab URL tests (pydata#1186)

Co-authored-by: JoerivanEngelen <[email protected]>

* Standardize template structure in more sections (pydata#1184)

* Standardize template structure in all sections

* Fixing footer behavior

* Update docs/user_guide/layout.rst

Co-authored-by: Daniel McCloy <[email protected]>

* Remove use of id= as much as possible

---------

Co-authored-by: Daniel McCloy <[email protected]>

* maint: remove sphinx-panels support; remove deprecated config shims (pydata#1188)

* Minor style improvements to ablog (pydata#1185)

* RLS: v0.13.0rc3

* dev0

* FIX: Some style bugs (pydata#1191)

* FIX: Some style bugs

* Move link word wrap to global rule

* DOCS: Add internationalization instructions (pydata#1178)

Co-authored-by: James McKinney <[email protected]>

* Refactor contributing docs to be more modular (pydata#1173)

* Dev0

* Fix github gitlab brand (pydata#1194)

* RLS: v0.13.0rc4

* FIX: Make wide equations scroll (pydata#1196)

* Fix math scrollbars for realz (pydata#1198)

* Fix math scrollbars for realz

* Update src/pydata_sphinx_theme/assets/styles/content/_math.scss

* Update src/pydata_sphinx_theme/assets/styles/content/_math.scss

* copy_logo_images: do not render dynamic Sphinx template content (pydata#1204)

* copy_logo_images: do not render dynamic Sphinx template content when copying logo image files

* Update src/pydata_sphinx_theme/__init__.py

---------

Co-authored-by: Chris Holdgraf <[email protected]>

* Add conditional check for last-updated template (pydata#1201)

* Add conditional check for last-updated template

* Whitespace

* Properly set configuration with app.builder.theme_options (pydata#1199)

* Properly set configuration

* Dict to values

* Making it explicit in a function

* Better name

* Fix test

* Foot

* Revert complex config set

* Clarify docs

* Use CSS transform for skip link (pydata#1206)

* feat: Add full i18n support (pydata#1192)

Co-authored-by: Daniel McCloy <[email protected]>
Co-authored-by: James McKinney <[email protected]>
Co-authored-by: Chris Holdgraf <[email protected]>
Co-authored-by: Chris Holdgraf <[email protected]>

* Dev0

* FIX: Remove icon links component when no icon links given (pydata#1209)

* RLS: 0.13.0rc5

* dev0

* FIX: Get theme options in a more robust way (pydata#1214)

* RLS: v0.13.0rc6

* Make heading-style use the font-weight-heading value (pydata#1213)

* Make heading-style use the font-weight-heading value

* Separate font-weight setting for content headers and admonitions

* Flip var to be consistent with --pst-font-weight-heading instead

* RLS: v0.13.0

* bump: dev0

* DOCS: Remove <p> from announcement sample text (pydata#1223)

---------

Co-authored-by: Chris Holdgraf <[email protected]>
Co-authored-by: Daniel McCloy <[email protected]>
Co-authored-by: Nico Albers <[email protected]>
Co-authored-by: Chris Holdgraf <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Brendan Heberlein <[email protected]>
Co-authored-by: Tania Allard <[email protected]>
Co-authored-by: Lunar <[email protected]>
Co-authored-by: Jean Abou-Samra <[email protected]>
Co-authored-by: Jérémy Bobbio (Lunar) <[email protected]>
Co-authored-by: JoerivanEngelen <[email protected]>
Co-authored-by: James McKinney <[email protected]>
Co-authored-by: James Addison <[email protected]>
Co-authored-by: Veronica Berglyd Olsen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants