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

versioning, picker component #178

Merged
merged 14 commits into from
Nov 16, 2024
Merged

versioning, picker component #178

merged 14 commits into from
Nov 16, 2024

Conversation

lazarusA
Copy link
Collaborator

No description provided.

@lazarusA
Copy link
Collaborator Author

Well, this is almost working, is EMPTY. What am I missing? @jkrumbiegel maybe is easy for you to spot the missing piece.

Screenshot 2024-09-18 at 09 37 11

@lazarusA lazarusA mentioned this pull request Sep 18, 2024
@jkrumbiegel
Copy link
Contributor

jkrumbiegel commented Sep 18, 2024

@lazarusA the versions.js file is not present at the expected location, for Makie it is there https://docs.makie.org/versions.js

image

Edit:

Ah it's at https://luxdl.github.io/DocumenterVitepress.jl/versions.js so the prefix is missing and needs to be added

@asinghvi17
Copy link
Collaborator

@lazarusA I think you can use deploy_url from the MarkdownVitepress object to get the correct / expected URL, and then simply do a string replace on the component if necessary.

head: [['link', { rel: 'icon', href: '/DocumenterVitepress.jl/dev/favicon.ico' }]],
head: [
['link', { rel: 'icon', href: '/DocumenterVitepress.jl/dev/favicon.ico' }],
['script', {src: '/versions.js'}],
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@asinghvi17 not here? I think I just need to add base here, isn't?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes that should also work!

Copy link
Contributor

Choose a reason for hiding this comment

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

doesn't base also contain dev or stable? If so that would not be correct

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, that doesn't work. It populates the menu, but the links are wrong.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Mmm, not sure how to propagate base to the Vue component. If I'm not too tired, I will check again later. It will be good to have this finally in the next release, which will be breaking either way. So, a big bump will also be needed. Someone that knows more about it, please commit the appropriate bump.

@lazarusA
Copy link
Collaborator Author

well, that last one worked. But, documenter is putting the wrong versions in the .js file.
https://luxdl.github.io/DocumenterVitepress.jl/versions.js

var DOC_VERSIONS = [
  "stable",
  "v0.1",
  "dev",
];

it looks like that, but it should be v0.1.1 a one is missing 😭 .

@tpoisot
Copy link
Contributor

tpoisot commented Sep 18, 2024

it looks like that, but it should be v0.1.1 a one is missing 😭 .

I think it's standard behavior, the major.minor version redirects to the latest patch version (that's how all my version.js files look)

Edit: yup - see https://github.com/JuliaDocs/Documenter.jl/blob/master/src/html/HTMLWriter.jl#L1476 - there's only one doc per minor release, so this seems to be working as intended 🎉

@jkrumbiegel
Copy link
Contributor

What's weird is that the link https://luxdl.github.io/DocumenterVitepress.jl/v0.1/ looks like it 404s, but it actually correctly resolves to the symlinked v0.1.1. The javascript console shows an error saying "Hydration completed but contains mismatches". When I refresh the page I actually see the homepage for a split second before the 404 overrides it. So vitepress seems not to be able to deal with the different address for some reason?

@tpoisot
Copy link
Contributor

tpoisot commented Sep 18, 2024

This seems relevant? vuejs/vitepress#4160

@lazarusA
Copy link
Collaborator Author

I'm re-working also the logic, so that local builds also work without errors.

@lazarusA lazarusA changed the title picker component, is this enough? versioning, picker component Sep 18, 2024
@lazarusA
Copy link
Collaborator Author

lazarusA commented Sep 19, 2024

I tested this on DD, without any issues. Here, most likely something went wrong when creating the v0.1.1 folder version.

https://rafaqz.github.io/DimensionalData.jl/previews/PR804/

}

const baseTemp = {
base: 'REPLACE_ME_DOCUMENTER_VITEPRESS',// TODO: replace this in makedocs!
Copy link
Contributor

@jkrumbiegel jkrumbiegel Sep 20, 2024

Choose a reason for hiding this comment

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

@asinghvi17 by the way, the way these strings are replaced makes the code unnecessarily ugly. Couldn't you give every bit its own name like REPLACE_ME_DOCUMENTER_VITEPRESS_FAVICON? Then one could it splice in directly wherever needed without relying on the base: thing being present.

Copy link
Collaborator

@asinghvi17 asinghvi17 Sep 20, 2024

Choose a reason for hiding this comment

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

We could, it's not super complicated to do either. Will look into it today.

@lazarusA lazarusA requested a review from avik-pal September 20, 2024 14:31
@avik-pal
Copy link
Member

In the Lux repo, only dev seems to show up https://lux.csail.mit.edu/dev/. Also there is a noticeable lag between the website being loaded and the time when the version picker appears. Any idea why that would be the case?

@lazarusA
Copy link
Collaborator Author

lazarusA commented Sep 22, 2024

yes, is not taking the custom domain, and in turn getting the right path for versions.js. The lag is due to the waiting time, I did put 5s before going int the default, dev. Something might be wrong with my logic to load versions from custom domains.

Edit: The culprit https://github.com/LuxDL/Lux.jl/blob/e23b1a73257ea7094e89f13d64b1cbd609d09c8b/docs/src/.vitepress/config.mts#L59

it should be just '/versions.js', but the {getBaseRepository(baseTemp.base)} is given dev, since it thinks that the versions file is at https://lux.csail.mit.edu/dev/versions.js. This was not considered, I haven't invested time figuring this one yet. Thanks for testing.

@avik-pal
Copy link
Member

it should be just '/versions.js'

This fixed it. Should we make a comment about that in the docs? Thanks for taking a look at this

@thofma
Copy link
Contributor

thofma commented Oct 10, 2024

Out of curiosity, what is the status of this? For my own project, shall I just copy it from https://lux.csail.mit.edu/dev/?

@lazarusA lazarusA merged commit 56ad88c into master Nov 16, 2024
2 checks passed
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.

6 participants