Reserve height for version picker note early on

[acolomb]

Split up the setVersionPickerOptions() function to create the admonition elements before the versions.json list was fetched. This will avoid the page jumping around when the GetJSON() promise takes some time to resolve.

Clean up some code styling along the way, using var instead of let and direct element attribute access instead of setAttributes().

[acolomb]

Note that the page still jumps a little bit during loading. That's because we need to wait for the DOMContentLoaded loaded event in order to find the place where to insert the box. But now the insertion (in hidden state, but taking the correct amount of space) happens as soon as possible. When the JSON file is loaded successfully, the box is simply switched to visible state. I couldn't find a way in JavaScript to avoid this small visual glitch.

The other option would be to move it statically to the layout.html file. But that needs rebuilding of all historic versions and is much more intrusive to the generated HTML files. Which we don't want to get into with past or future theme changes.

For testing, this currently only applies to the "latest" version in the preview. So the difference in load times can be seen when switching between that and older versions.

[tomasz1986]

I've tested the fixed version. I will say that the situation is much better now, as there's no noticeable content move on load.

However, I'm wondering, is there any reason not to keep the version picker visible from the very beginning? I've tested this locally, and doing so prevents the grey background from flashing on each page load. Of course, the picker itself remains non-functional until everything finishes loading, but still, the user experience, at least for me, seems better and much less distracting without the "flashing".

[acolomb]

The simple answer is that I see the built HTML documentation as a self-contained collection of documents which could be used offline. If you only have one build without a version.json file in the right relative path, then the whole thing is not even shown in the first place.

[imsodin]

Conceptually I believe we should design for the main use-case - here the docs website. I.e. I'd rather have the bar by default and have it usuless in the local scenario than some flashing effect or something on the website. I'd rather invest into hiding it on scroll down or something.

[acolomb]

The version picker bar is supposed to be present on all published versions. So if we were to include it statically in the HTML, there would be no flashing, but every single built HTML file of every archived version needs to be rebuilt. That's what I wanted to avoid by inserting it from JavaScript, but to find the correct location we need to wait for the document to finish loading the DOM first. That's where the flashing comes from, because browsers don't delay displaying.

I'd rather invest into hiding it on scroll down or something.

What do you mean? It scrolls off the viewport normally right now when you go further down in the document.

[tomasz1986]

That's where the flashing comes from, because browsers don't delay displaying.

I haven't tested in other browsers, but at least Chromium seems to cache it even when inserted via JS. It only happens if the element is always visible though. With the solution proposed in this PR currently, which sets its visibility to hidden first, and only changes it to visible later, there's flashing. On the other hand, if there's no visibility switching going on, there's also no flashing, and the browser makes it seem as if the element was just fixed there even when switching between different docs pages.

However, I'd say "flashing" is still way better than having the whole content move as it's the case right now 😉.

[imsodin]

[...] to find the correct location we need to wait for the document to finish loading the DOM first.

Ok, thanks for clarification. My understanding of the previous info was that it needs to wait for the versions.json only.

And following what @tomasz1986 says, if that does indeed help to remove the flashing, I'd also advocate for having it visible by default and only hiding if there's nothing to switch (or not hiding at all, just showing the version and disabling the drop-down or something).

Generally if both @tomasz1986 and you are happy with this change, please don't delay going ahead on my behalf - the code changes themselves look good.

What do you mean? It scrolls off the viewport normally right now when you go further down in the document.

Sorry about that, very inaccurate and far-fetching projection from my side (header-like -> those usually take up vertical space -> no good), completely ignoring the reality of what you did.

[acolomb]

Ok, thanks for clarification. My understanding of the previous info was that it needs to wait for the versions.json only.

On the contrary, that happens even later than loading the full HTML file.

And following what @tomasz1986 says, if that does indeed help to remove the flashing, I'd also advocate for having it visible by default and only hiding if there's nothing to switch (or not hiding at all, just showing the version and disabling the drop-down or something).

I'm not quite sure what that "flashing" looks like exactly. The current code waits until the HTML is complete, then takes up the needed space using an invisible box, and only if the versions.json can be loaded successfully afterwards, switches it to be visible. If it appears temporarily before that, it must be a rendering glitch in the browser. Because I am setting the visibility: hidden attribute before even adding it to the DOM at the right location.

Adding it in visible state, then hiding can only make things worse, because then it would rightfully appear for a moment. With the current approach, if it does appear, that's a browser funkyness. But maybe I'm misunderstanding what "flashing" you mean exactly.

We could display the grey box always, but hide its content until certain it will work? It will still show up later than the rest of the page (inevitably), but the interval may be so short it won't catch attention I suppose. EDIT: Committed just that as an experiment, let me know what you think @tomasz1986. I don't like the potentially empty box :shrug:

[acolomb]

@tomasz1986 one more look at the latest design change? I'm inclined to go back to not showing an empty box, but hide it completely. Scream if that state is not acceptable :wink:

[tomasz1986]

I've just re-tested the Netlify build. I would suggest to keep the empty box. It feels better this way, because only the text is refreshed when navigating between different pages, while the box itself stays intact. It's just less distracting this way 🙂.

[acolomb]

Okay I just fixed one remaining issue, so that the whole box gets hidden again when the version picker doesn't actually work. So we don't end up with an empty box, but it just possibly flashes up shortly. Only in the unlikely event of failure (such as when deploying locally).

@tomasz1986 do you agree with keeping the box content centered?

[tomasz1986]

@tomasz1986 do you agree with keeping the box content centered?

Having it centred does look better for me, at least with the current theme, as it matches the position of the main body text.

[acolomb]

Thanks for all the feedback. Let's drive this home finally.