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

Make the theme responsive #46

Open
wants to merge 3 commits into
base: master
from
Open

Make the theme responsive #46

wants to merge 3 commits into from

Conversation

@obulat
Copy link

@obulat obulat commented May 19, 2020

I have been reading on my phone a lot lately, and was disappointed by the fact that I couldn't read python docs: the fonts are too small, and, if I try to zoom in, the text doesn't fit on the page, so I have to swipe to see the whole line.

This is my attempt to add responsiveness to python docs theme, fixing #30.

On screen widths smaller than 900 px, it:

  • adds a mobile navigation bar with a 'hamburger' menu button, Python version switcher and a search box.

  • adds a sliding menu that opens when 'hamburger' menu button is clicked. Menu contains language switching input and contents.

  • removes the related bars

  • removes the sidebar.

Screenshot with menu closed:
Responsive  Menu closed Galaxy S5
Screenshot with menu open:
Responsive  Menu open Galaxy S5

Problems that need to be solved:

  • Normally, the mobile navigation bar should be placed in the header block of the layout, but python.org layout completely overrides it, so I added it in the body_tag block. To solve this, I would need to add changes to that code.

  • I placed the switchers and other items on the menus as seemed logical to me, but feedback from the community is essential on this.

  • I added a javascript file to open/close sidebar in a separate file. I could also add it into the layout file, but ideally all js files should be combined and minified.

Copy link

@septatrix septatrix left a comment

Overall a great idea and I would also really like a responsive theme though there seem to be some unrelated changes and implementation details which should be discussed

{% endif %}
{{ super() }}
{% endblock %}

{%- block css %}
<meta name="viewport" content="width=device-width, initial-scale=1.0">

This comment has been minimized.

@septatrix

septatrix Jun 14, 2020

This got added in sphinx 3.1 and should work mostly without problem for people using old sphinx versions. May however not hurt to include it for now and delete it later

This comment has been minimized.

@obulat

obulat Jun 14, 2020
Author

I added this to sphinx after creating this PR.

python_docs_theme/layout.html Outdated Show resolved Hide resolved
python_docs_theme/layout.html Outdated Show resolved Hide resolved
* {
box-sizing: border-box;
}
Comment on lines 3 to 5

This comment has been minimized.

@septatrix

septatrix Jun 14, 2020

This should not be necessary and may break existing layouts

This comment has been minimized.

@obulat

obulat Jun 14, 2020
Author

OK, I will remove this.

python_docs_theme/static/pydoctheme.css Outdated Show resolved Hide resolved
python_docs_theme/static/pydoctheme.css Outdated Show resolved Hide resolved
.nav-content .search label {
border: 0;
clip: rect(0 0 0 0);
height: 1px;
margin: -1px;
overflow: hidden;
padding: 0;
position: absolute !important;
width: 1px;
}
Comment on lines 290 to 299

This comment has been minimized.

@septatrix

septatrix Jun 14, 2020

What exactly is the purpose of the label if you try to hide it (at least that what it seems so far from this code)

This comment has been minimized.

@obulat

obulat Jun 14, 2020
Author

This label is there for accessibility reasons: screen readers can read it, but it doesn't crowd the layout. The search box is implemented this way on mdn website.

This comment has been minimized.

@septatrix

septatrix Jun 14, 2020

Ah I see, so this is a manual version of sr-only like in bootstrap. I would probably just use a aria-label="{{ _('Quick search') }}" on the input element though

python_docs_theme/theme.conf Outdated Show resolved Hide resolved
document.addEventListener('DOMContentLoaded', function () {
const toggler = document.querySelector('.toggler');
const sideMenu = document.querySelector('.menu-wrapper');
const doc = document.querySelector('.document');
function closeMenu() {
sideMenu.classList.remove('open');
toggler.checked = false;
}
toggler.addEventListener('change', function (e) {
if (toggler.checked) {
sideMenu.classList.add('open');
} else {
closeMenu();
}
});
doc.addEventListener('click', function () {
if (toggler.checked) {
closeMenu();
}
})
})
Comment on lines 2 to 22

This comment has been minimized.

@septatrix

septatrix Jun 14, 2020

By moving the checkbox up in the dom tree it may be possible to implement this using only javascript and make it more accessible for people without javascript

This comment has been minimized.

@obulat

obulat Jun 19, 2020
Author

I replaced the checkbox with a button. Is this better for people without javascript?

This comment has been minimized.

@septatrix

septatrix Jun 20, 2020

What I would have done is move the input as a checkbox directly next to the sidebar, reference it in the label via id and apply the styles based on whether the checkbox is checked or not. I may have time tomorrow to create a PR on your fork repo if you want to.

This comment has been minimized.

@obulat

obulat Jun 21, 2020
Author

I have done it like this at first (checkbox input and applying styles based on :checked property). Then I was checking for accessibility and found this article which shows that a button as menu opener has some advantages regarding accessibility: namely, it is easier to set up keyboard handling. On the other hand, I just realized that people probably don't use keyboard for navigation on mobile, and it is more important to set up No-script solution than keyboard navigation on mobile.
Also, I looked at other documentation sites: readthedocs theme and VueJS docs, and their hamburger menu doesn't work without javascript.

This comment has been minimized.

@septatrix

septatrix Jun 23, 2020

In addition if you follow this guide you also get a working css only solution with added accesibility using javascript

@obulat
Copy link
Author

@obulat obulat commented Jun 14, 2020

Thank you for your review, @septatrix !
I don't have a reliable internet connection at the moment, but will try to address your comments as soon as possible. Overall, I have tried to make the theme accessible as well as responsive, that's the reason for some label handling. Also, I tried to use only Vanilla javascript to make it easier to remove jquery dependency in the future.

I would also like to have your feedback on the content for the mobile version. For example, the top navbar on mobile: Do you think logo, version switcher and searchbox are what should be there, or do we need to add/ remove something?

Also, the relbar is completely removed, but I thought that it might be a good idea to add it at the bottom, without the searchbox, though.

- Remove unnecessary `show` call for searchbox.
- Remove `box-sizing:border-box` for all elements.
- Add `aria-label` to search input instead of visually-hiding labels for accessibility
- Some style fixes
@JulienPalard
Copy link
Member

@JulienPalard JulienPalard commented Jun 20, 2020

Beware, i'm working on moving the switchers generation code to docsbuild scripts instead of cpython. See python/docsbuild-scripts#90

I did not think about switchers being located in different places, we have to check if there is something to fix either in docsbuild side or here.

As the switchers are no longer to be maintained in cpython, I was removing the placeholders and the switchers variable in python/cpython#20969

I'm on my phone right know so I can't test it, but we need to agree on how to "communicate" to docsbuild the switchers places.

@JulienPalard
Copy link
Member

@JulienPalard JulienPalard commented Jun 21, 2020

Hi @obulat!

Took the time to think about it, but beware, I'm particularily bad at HTML/JS:

  • python-docs-theme is not cpython specific, it can be used elsewhere, that's typically why the "This document is for an old version of Python" header is in cpython and not in the theme.
  • Switchers are not cpython specific: other documentations can be translated and have versions.
  • The classes .version_switcher_placeholder and .language_switcher_placeholder has always been in use and looks good to me.
  • Having multiple version placeholders may work, IIRC, jquery will properly create them all when I do $('.version_switcher_placeholder').html(version_select);

The current switchers.js, the one from docsbuild-scripts has two ways of working (see function create_placeholders_if_missing):

  • Either there's a '.version_switcher_placeholder' and it uses it (typically when cpython or the theme places it)
  • Either there's not, and it tries it best to find a place to create both placeholders (side by side), see the ugly probable_places array (I may soon add a cleaner cpython-language-and-version in front of it, but it may not be usefull if you handle it in the theme).

What about adding all needed placeholders in python-docs-theme, so switchers.js from docsbuild-scripts could rely on them, instead of trying to inject HTML? In "mobile version" it currently injects them in HTML that is not rendered, so no switchers appear.

How to test with docsbuild-script locally:

$ # In a clone of docsbuild-scripts
$ mkdir -p www logs build_root
$ python3 -m venv build_root/venv/
$ build_root/venv/bin/python -m pip install -r requirements.txt
$ build_root/venv/bin/python -m pip install path_to_your_python-docs-theme
$ python3 ./build_docs.py --quick --build-root build_root --www-root www --log-directory logs --group $(id -g) --skip-cache-invalidation --language en --branch master

(Currently testing it and wow, that's really nice, thanks a lot for working on this topic!)

@obulat
Copy link
Author

@obulat obulat commented Jun 22, 2020

Hi, @julien, thank you for your comments, that's really helpful!

I will try to look into it as soon as I have time.

@septatrix
Copy link

@septatrix septatrix commented Jun 23, 2020

I think a problem which should be tackled first is overflowing stuff like tables, all dt for definitions as well as code segments inside the docs. These are for the most part not broken an lead to a wider page than intended. Together with the addition of the <meta name="viewport" ...> tag this will already solve most problems. This way no major design changes would be necessary as the sidebar should actually also work great on mobile devices (maybe make it a few pixels wider). The problem with the header/footer wrapping badly due to its floating nature already happens for me at around 1200px width.

- Menu is opened/closed using only css (invisible checkbox), works without js enabled
- Accessibility added using javascript
@obulat
Copy link
Author

@obulat obulat commented Jul 12, 2020

I think a problem which should be tackled first is overflowing stuff like tables, all dt for definitions as well as code segments inside the docs. These are for the most part not broken an lead to a wider page than intended. Together with the addition of the <meta name="viewport" ...> tag this will already solve most problems. This way no major design changes would be necessary as the sidebar should actually also work great on mobile devices (maybe make it a few pixels wider). The problem with the header/footer wrapping badly due to its floating nature already happens for me at around 1200px width.

I've added wrapping tables in a div that can be horizontally scrolled: this is the solution used in readthedocs. dt and code segments are already horizontally scrollable.

}
/* Typography */
div.body h1, div.body h2, div.body h3, div.body h4, div.body h5, div.body p {
font-family: SourceSansProRegular, Arial, sans-serif;

This comment has been minimized.

@septatrix

septatrix Sep 20, 2020

Why do you change the font family for a specific width?

z-index: 1;
}
.mobile-nav * {
box-sizing: border-box;

This comment has been minimized.

@septatrix

septatrix Sep 20, 2020

If possibly I would stay with the sphinx default of not modifying the box-sizing property

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.