Skip to content

Sticky headers - #504

Open
danny-brandt wants to merge 4 commits into
mainfrom
sticky-headers
Open

danny-brandt wants to merge 4 commits into
mainfrom
sticky-headers

Conversation

@danny-brandt

@danny-brandt danny-brandt commented Oct 8, 2026 •

Copy link
Copy Markdown
Collaborator

Closes https://github.nrel.gov/TADA/afdc/issues/3239
Makes request and response header for alt-fuel-stations docs sticky

@danny-brandt
danny-brandt requested a review from mrahill October 8, 2026 17:29
@danny-brandt danny-brandt self-assigned this Oct 8, 2026

@GUI GUI left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for looking into this! There are some issues with the content all being indented messing up Markdown parsing, so we'd need to revisit the content more closely to fix all of those issue. But I was also wondering if we actually need the wrapper div at all that led to this, so maybe we could get rid of it, and then these Markdown issues would go away?

I had some other thoughts on standardizing the styles so we're not introducing custom styles. The overall look seems like it maybe blends in with the existing code blocks and table headers, but maybe that's okay? I don't have super strong feelings on the look and feel.

There's also some existing production issues with the none of our table-based docs being fully viewable on mobile devices, but that's probably outside the scope of this PR.

Comment on lines +8 to +11
html,
body {
overflow-x: clip;
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since this is unwinding a default NREL template setting, can you shift this to frontend/stylesheets/_nlr_app_template_overrides.scss, and add a comment explaining this is for sticky headers?

Also, this is an existing production issue, but the default overflow-x: hidden, and this new overflow-x: clip make various parts of our docs that use tables inaccessible on mobile devices. Because some of our tables are too wide, these overflow settings prevent mobile users from seeing the content altogether (rather than being able to scroll). While our wide tables maybe aren't ideal, that may be a bigger issue to fix. So the quickest fix would be to use overflow-x: unset to allow side-to-side scrolling, since while it's not ideal for mobile users, it's at least better then hiding the content and making it completely inaccessible. However, that introduces its own set of issues, since it looks like there are things in the template that then force the entire page to 4000px wide with that removed, so we'd have to sort all of this out:

#app-menu, #program-menu:before {
content:"";
background-color: #5e6a71;
position: absolute;
height: 100%;
width: 4000px;
left: -2000px;
z-index: -1;
}

Comment on lines +12 to +14
.sticky-container {
height: min-content;
position: relative;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is these sticky-container wrapper necessary? I couldn't really tell what it does differently when it's present versus not.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The sticky container creates a definitive end for the header. If we take it off and scroll down to the "JSON Output Format" section the "Request Headers" header will continue to stick as you scroll down. I am fine taking it Off if that behavior is ok, but I figured we would want it to stop at that point.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I guess the CSS itself is not necessary but the

creates the endpoint
This is without the div

Screen.Recording.2026-10-29.at.4.53.53.PM.mov

Comment on lines +16 to +25
.sticky {
z-index: 1000;
position: sticky;
top: 0;
background-color: #eee;
padding: .4em .6em .3em .6em;
margin-bottom: .5em;
border-bottom: 1px solid rgba(0, 0, 0, 0.08);
box-shadow: 0 1px 0 rgba(255, 255, 255, 0.35) inset, 0 1px 3px rgba(0, 0, 0, 0.05);
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we replace this custom class with a Bootstrap utility classes? These aren't using standard colors used elsewhere in the template, so rather than introducing ad-hoc colors and spacing, I'd like to use the existing colors for better standardization. For example, this isn't an exact match (I'm skipping some things like the shadow, since I couldn't tell much of a visual difference with it), but I think this could mostly be replicated with (but we could play with other utility helpers):

<h2 class="sticky-top p-2 bg-body-secondary border-bottom">

Comment on lines +18 to +20
<div class="sticky-container">
<h2 class="sticky"> Request Parameters</h2>
The following query parameters may be passed in the request to control the output:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

By introducing this sticky-container wrapper div and indenting everything, it means the contents inside are no longer parsed as Markdown. Any contents like this then can only be HTML. So there are various new issues where we'd have to replace Markdown things with explicit HTML markup. For example, this "The following query..." text is no longer wrapped in a <p> tag, which alters the spacing, and there are other headers and paragraphs missing markup, like these headers and paragraphs below:

Image

But back to one of my earlier comments, do we actually need the sticky-container div? If not, that may simplify all of this, since then this could all go back to how it was, and we'd only be introducing custom <h2> elements.

This branch has not been deployed

No deployments
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.

2 participants