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

Whole-document visible deprecation #2719

Open
Yhg1s opened this issue Jul 19, 2022 · 13 comments
Open

Whole-document visible deprecation #2719

Yhg1s opened this issue Jul 19, 2022 · 13 comments
Labels
infra Core infrastructure for building and rendering PEPs meta Related to the repo itself and its processes

Comments

@Yhg1s
Copy link
Member

Yhg1s commented Jul 19, 2022

While discussing the question of updating or superseding/deprecating PEPs, it occurred to me that it would probably be a good idea to display PEPs that aren't active (have been superseded, rejected or otherwise no longer apply) in a way that their status is more apparent than just the one header line at the top, or (like PEP 302) a single note section at the top. If we could have a background text, background colour, floating notice on the side or any kind of visual indication that the text is not authoritative (anymore) even when following a direct link to a section in the PEP, that would be great.

@warsaw
Copy link
Member

warsaw commented Jul 20, 2022

To generalize that, what about adding some kind of emoji to each line in the PEP 0 index? It should describe a different dimension/status than the first column, and we'd have to come up with a taxonomy that makes sense, but it could help gather more information about the PEPs from glancing at the index. (Of course, that emoji could also be carried over into the individual PEPs, and would likely be represented by some metadata tag in the PEP text itself.)

@brettcannon
Copy link
Member

The indicator I thought of is when you have a PDF or something that has big, red, translucent "DRAFT" wording on a slant in the background.

@CAM-Gerlach CAM-Gerlach added infra Core infrastructure for building and rendering PEPs meta Related to the repo itself and its processes labels Jul 22, 2022
@CAM-Gerlach
Copy link
Member

Instead of a new bespoke mechanism, at least for now I suggest just adding new variant(s) of the warning banner implemented in #2702 with appropriate content for each case, and implementing the followup proposal to make it "stick" to the top of the page would make it always visible and very hard to miss, unlike a regular admonition. That approach has the advantage of being flexible and extensible enough to cover the full variety of potential cases here while being prominent, informative and not too disruptive.

To generalize that, what about adding some kind of emoji to each line in the PEP 0 index? It should describe a different dimension/status than the first column, and we'd have to come up with a taxonomy that makes sense, but it could help gather more information about the PEPs from glancing at the index.

Instead of inventing a new dimension, since PEP 1 already displays status codes and groups PEPs by it, we could just introduce an Obsolete status, for PEPs that were Accepted/Final but now only of historical relevance, which would take advantage of the existing status codes and category grouping...though isn't this really true of nearly all Final PEPs, since for Standards-Track PEPs it means they are historical documents their canonical documentation is to be hosted elsewhere, and for Informational and Process PEPs it means they are no longer Active and being updated? We could also adjust the category index tables to be more useful in this regard.

@hugovk
Copy link
Member

hugovk commented Jul 22, 2022

This CSS can be adapted to put rotated background text, but we'd need to put something like class="rejected" in the <body> tag for the relevant PEPs, and I'm not sure how to do that.

image image

So a sticky #2702 banner is likely easier.

@CAM-Gerlach
Copy link
Member

It could either be hacked around with JS and applying a custom directive or adding the class to some specific body element, or something could be done at build time when reading the appropriate header, but a banner is still very visible while being less disruptive to legitimate readers (since the PEPs still have historical value), we'd have to decide which PEPs qualify and authors might not be too happy about their PEPs being defaced—the banner also provides useful information and a recommended alternative to readers rather than just a blaring warning.

@pradyunsg
Copy link
Member

pradyunsg commented Nov 19, 2022

FWIW, it's "fairly" straightforward to have a banner that stays on top of the page. If we pick #2702's banners, it'd be a matter of:

  1. Adding the following CSS:

    .pep-banner {
       top: 0;
       position: sticky;
    }
  2. Somehow deal with the fact that :targets (i.e. anchor tags) will need to have a scroll-margin-top that matches the hieght of the banner. Either with an offset that's always present (which isn't the worst idea) or with an offset on pages that have the banner.

@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Nov 19, 2022

Oh nice, I didn't expect it to be that simple, since I've had to deal with some fairly complex sticky top bars, but then again it's probably because I'm not an expert like you :).

Better yet, we should add a :sticky: option to the banner directive to easily control whether a certain banner is sticky, which if set adds "sticky-banner" to self.css_classes, and add the CSS snippit as above (except with sticky-banner instead of pep-banner). Banner subclasses (like the PyPA banner) could include sticky-banner in their custom their css-classes to be sticky automatically (or we could also do that directly in CSS).

The following JS snippit executed on load should handle the scroll offset:

stickyBanner = document.getElementsByClassName("sticky-banner")[0]
if (stickyBanner) {
    style = document.createElement("style")
    style.innerHTML = "[id] { scroll-margin-top: " + stickyBanner.offsetHeight + "px; }"
    document.head.appendChild(style)
}

Only issue is the scroll offset won't adapt to the banner being taller if the viewport is resized to be much narrower after load fires (assuming we execute the snippit then). We could get around that by adding it as an event handler to e.g. resize to update the offsetHeight, but given that's probably not too common, I doubt that's worth it in terms of complexity or performance when just adding an extra fixed offset of like 3 rem to the scroll-margin should avoid it for most cases.

scroll-margin-top

Oh, somehow I never knew that existed and just hacked together a bunch of JS to do it instead (which was really finicky to get working reliably cross-browser/device). TIL. Though, at the time (several years ago) browser support for it was not as good as it was now, so I might not have been able to rely on it anyway.

@pradyunsg
Copy link
Member

The following JS snippit executed on load should handle the scroll offset:

It might need to be a render-blocking JS (for anchor links to have the offset on first render -- I don't know when the browser does the actual scroll though). That isn't the worst thing ever, but may be possible+valuable to avoid?

Thinking about this a bit, we may be able to add a class to the body or some other wrapper which contains the sticky banner, and keep the scroll-margin-top in css; as .enable-sticky-banner [id] / .enable-sticky-banner .pep-banner.

@CAM-Gerlach
Copy link
Member

It might need to be a render-blocking JS (for anchor links to have the offset on first render -- I don't know when the browser does the actual scroll though).

Oh, right—I recall having to deal with the specifics of this when I was implementing a JS-based solution to this a few years ago before scroll-margin-top was widely supported, but I can't remember the exact details off the top of my head. It shouldn't be that big of a deal, though.

Thinking about this a bit, we may be able to add a class to the body or some other wrapper which contains the sticky banner, and keep the scroll-margin-top in css; as .enable-sticky-banner [id] / .enable-sticky-banner .pep-banner.

Yeah, I thought about that too, but the problem is there would be no way to know before the page is rendered what to actually set the scroll margin to, since the directive height could vary substantially depending on its contents, the current viewport width and other factors. I also don't know how easy that would be to add the class to the root document node from inside a directive run function, though I figure its possible (I'm just not familiar with how to do it).

@pradyunsg
Copy link
Member

Spending some time thinking about this, I think the idea of having a page wide status text in the background is a good way to achieve the goal of avoiding user confusion.

The goal of #2992 is to make it easier to see the banners we've added, and showing the status via colours and background text should help as well.

The tricky thing, of course, is figuring out if we can do it in a reasonably accessible manner.

@hugovk
Copy link
Member

hugovk commented Feb 18, 2024

See PR #3682 to add directives to add a sticky warning banner.

@hugovk
Copy link
Member

hugovk commented Apr 14, 2024

Merged, but before adding the next batch let's check #3682 (comment) and #3756 (review): do we need the blank line, or can we modify the custom directives somehow?

@pradyunsg
Copy link
Member

I've filed #3765 if we want to try to modify the custom directives to use an option instead of an argument for the related links (there's one bikeshedding item in there).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infra Core infrastructure for building and rendering PEPs meta Related to the repo itself and its processes
Projects
None yet
Development

No branches or pull requests

7 participants
@brettcannon @warsaw @hugovk @pradyunsg @Yhg1s @CAM-Gerlach and others