Implement support for sticky banners and utilize them #2992
Conversation
These banners are now sticky, ensuring that they are visible throughout the document.
There was a problem hiding this comment.
Thanks @pradyunsg for helping out on this! I'd meant to get to this previously, but it just slipped through the cracks.
In testing this, sometimes when loading a page with a fragment identifier (the original scenario motivating this change), at least for me in FF on Windows, the scroll offset isn't applied and the banner covers the heading, which I'm guessing is because the scroll event races this snippit on DOMContentLoaded and sometimes the scroll is executed before the scroll offset has been set.
To avoid this, you could add something like the following at the end of the JS snippit:
if (location.hash.length > 1) {
document.getElementById(location.hash.substring(1)).scrollIntoView();
}I'm also experiencing the offset height sometimes being too much on Firefox mobile on a Pixel 3a latest Android, at least on first uncached page load, whereas subsequent cached loads fixes it. I'm thinking this might be due to the stylesheet/other assets not being already fully loaded and rendered that results in a different calculated height for the banner at the time of DOMContentLoaded than eventually gets applied, which changing it to load might fix if so.
pep_sphinx_extensions/pep_processor/parsing/pep_banner_directive.py
Outdated
Show resolved
Hide resolved
pep_sphinx_extensions/pep_processor/parsing/pep_banner_directive.py
Outdated
Show resolved
Hide resolved
|
Thanks for this! It's working on mobile too, but takes up quite a bit more space. On my Samsung S10:
Maybe once it becomes sticky, it could shrink and show a shorter message? This doesn't happen on my phone, but does when testing other layouts using desktop Chrome (e.g. iPhone SE and Pixel 5): Click on a header to go to https://pep-previews--2992.org.readthedocs.build/pep-0621/#rationale and the Rationale header text is obscured by the sticky header:
Finally, this might just be a problem with desktop Chrome's mobile renderer, but there's a single line of pixels shown above the sticky header, showing a tiny slice of the body text:
As you scroll, that pixel line flickers as the text goes past. The fix would be to change |
|
Thanks for the detailed further testing and screenshots!
Hmm, well that would require more extensive changes, and wouldn't serve the primary use case of making it sticky (so it shows up when deep-linking inside PEPs) as users would never see the full thing. What about adding a close button (
This is because the sticky height is only calculated once on page load, so unless you do a full reload after switching device profiles, the height won't update. I naturally see this also on FF desktop's mobile renderer, but it goes away when reloading, and won't happen on a real device, of course (and I can also confirm it worked, with the one sporadic issue above, in both FF and Chrome on my Pixel 3a).
I can't repro this in either Chrome or FF on my Pixel 3a, nor on FF desktop's mobile renderer with the profiles of the devices you mentioned (iPhone SE and Pixel 5). I strongly suspect this just has to do with the precise framing of the simulted mobile viewport not exactly matching what would be seen on a real phone's screen. |
Can't reproduce this on BrowserStack's "real devices" rendering -- so... I'll say this is a bug in Chrome. :) |
|
@pradyunsg what do you think about
|
I like the sound of this -- I think it's OK for us to do this in a follow-up, because that's a significant amount of complexity on its own. |
I think the close button is good-enough as a solution for this TBH, because it's a more respectful way to allow users to hide banners as-and-when they wish to do so. |
I've tried multiple attempts at reproducing this and failed to do so. I've also added a One thing that came up in manual testing for this was the need for handling resizing, and I've logic to cover that as well. TBH, I'm leaning toward good-enough + done is better than perfect + not-done -- I suggest that we improve this further in follow ups if there are not show-stopping concerns here; rather than having this go through multiple cycles to do it perfectly in the first cut. :) |
|
Thanks for all your help with implementing this and getting this over the line, @pradyunsg , as well as putting up with all our nitpicks and review feedback. I look forward to using this feature in the days ahead. |
|
Looks like this is causing a problem for PEP 12 only, in that it creates an offset for a banner that doesn't exist. To reproduce online:
To reproduce locally: make clean dirhtml
python -m http.server -d build
Can reproduce with |
My motivation with this is to make the Packaging PEPs more clearly point readers to the canonical location (i.e. packaging.python.org) rather than the PEPs that introduced them. :)
This also makes some progress toward #2719, by implementing one of the approaches discussed there.
PEP 621 is a good PEP to try to see how this behaves: https://pep-previews--2992.org.readthedocs.build/pep-0621/