mhkeller
Migrate Layer Cake website to Svelte 5
This migrates the website to Svelte 5, components remain as before. So this is step 2 as discussed in https://github.com/mhkeller/layercake/issues/156#issuecomment-2722042331
(This should come after adding more tests from #261 and #270, i've run them only locally.)
Wow thanks for all of this! Should we convert these PRs to drafts to better track which ones are done?
https://github.com/mhkeller/layercake/pull/261 and https://github.com/mhkeller/layercake/pull/270 are done but I think we still want to do https://github.com/mhkeller/layercake/pull/276 before tackling this one
Noting that this PR and https://github.com/mhkeller/layercake/pull/281 will have to be merged into main together.
There should probably be a test for the graphics on the landing page, too.
Not sure if you noticed already that the Beeswarm plot is not looking right. (When you click through it's alright.)
There are a lot failing due to minor pixel differences. But there are others those where the reactivity is broken. Here's one example where I fixed it by cutting out intermediary derived values: https://github.com/mhkeller/layercake/pull/271/commits/51f556b4161dbb400aba980e75a30bab92bb0fe6
The migration script added in a temporary run function to some of the components so those are the ones I'm tackling first.
Once the obviously broken ones are done, maybe we re-run the screenshots to fix the minor differences.
There should probably be a test for the graphics on the landing page, too.
Not sure if you noticed already that the Beeswarm plot is not looking right. (When you click through it's alright.)
I didn't see your message before I posted that comment. Yep I think that falls under the category of "broken reactivity." Generally if the chart is bunched up into the top left corner, it means it rendered server-side but never got updated $width and $height values on the client.
but yes, it would be a good idea to add a home page test. Probably best to add that as a new PR using the current main snapshots as a baseline
Yep I think that falls under the category of "broken reactivity." Generally if the chart is bunched up into the top left corner, it means it rendered server-side but never got updated
$widthand$heightvalues on the client.
Yeah, great that you found a way to fix that! For the landing page example I don't know why it's failing since it's loading the component which looks alright on it's separate page.
but yes, it would be a good idea to add a home page test. Probably best to add that as a new PR using the current main snapshots as a baseline
I can do that.
Awesome. After removing run now it breaks in both places, so it's at least consistent. This one is definitely weird. One thing to be aware of is the migration command added snippets in some places but the main library doesn't support that yet. Ideally we could get all of the components working properly without rebuilding the main LayerCake.svelte component
I updated a few of them but I'm running into an issue where
- Canvas components and examples are running into an infinite loop, likely due to issues raised here: https://github.com/mhkeller/layercake/issues/250
- So I tried commenting out the canvas examples and components but
npm run buildstill fails because the prerender step is looking for those pages. So I added this in the svelte config
prerender: {
handleHttpError: 'warn'
}
Next step is to continue going through components and examples and getting them to match the screenshots.
I've also updated some of the types so those snapshots will also need some updating. I removed some undefined types when the prop was already optional.
@rgieseke I think I messed up the linux screenshots in trying to reset them. What's the command for rebuilding them and committing them in CI? It would probably be a good idea to put together a document describing the new tests and workflows and when to use them.
@rgieseke I tried manually running the update snapshots workflow but I get this error: https://github.com/mhkeller/layercake/actions/runs/16391856225/job/46318879345
@rgieseke I tried manually running the update snapshots workflow but I get this error: https://github.com/mhkeller/layercake/actions/runs/16391856225/job/46318879345
I think the code doesn't handle multi-line strings properly. My PR test was only with a single changed component only.
https://docs.github.com/en/actions/reference/workflow-commands-for-github-actions#multiline-strings
Maybe
echo "files<<EOF" >> $GITHUB_OUTPUT
echo "$files" >> $GITHUB_OUTPUT
echo "EOF" >> $GITHUB_OUTPUT
Maybe it's also better to check for png files in the snapshot directory only, would skip the grepping logic.
git status --porcelain test/e2e/chart-screenshots.spec.js-snapshots/ | awk '{print $2}'
Not sure whether the ARIA Yaml files should be handled as well in the CI action.
FYI the above commit now has the REPL links being created dynamically. It still needs some work standardizing the import paths since the REPL doesn't have any folders. But it will be a huge time save and ensure that the REPL links always match what's on the website. Unlike before, the URLs won't be stable, it's just encoding the state of the example in the URL hash.
One side effect of this approach is it makes less sense to include edit button on the home page because it requires loading all of the components. An alternative could be to pre-compute it in a build step or something but the hashes themselves are quite long.
Another alternative is to have some kind of playwright script visit the hash, login with saved credentials, save it, then copy that shorter URL.
I think what we have here is a pretty good first version and we can adjust unless anyone wants to dive in with another way to do it.
I was running into an error in the REPL when using the new async mode that I think comes from putting setContext in a reactive statement. I removed that here since I don't think this call is necessary since the stores will be reactive and the context does not need to be reset. But I want to keep my eye on it to make sure this didn't cause any issues.
@rgieseke I'm having a hard time seeing the artifacts for this failed test. Are you able to see what's wrong with it? I think it's on the new gallery image.
The problem was I removed the edit button from the landing page. I think @rgieseke you have to rebuild the snapshots for this branch via the action page on your fork. When I try to do it, it updates my version of that branch and getting them to merge is quite weird.
I fixed it by checking out a new branch based on this one, running the action on that and cherry picking the commit. Not ideal but it works. This needs some testing / clicking around to make sure it's all good.
Awesome! I've been checking this out but will give it a further test. I saw that a pnpm lock file was committed. I assume that was unintentional?
Good catch – removed. I'm doing a tutorial tomorrow on layercake so I'll likely be merging these this evening so the material is up-to-date for that but testing can of course continue after that.
Yes, absolutely - I didn't spot anything so far. Very good solution with the Playground links!
Not sure if it's necessary to include the Darwin screenshots. I'd think what's in the repo should be for CI. There might be someone on Windows who then needs different screenshots and I'm not sure how stable Darwin screenshots are between different macOS versions. Fine to keep them of course if it makes maintenance easier for you! Or just something we need to add to the Development docs.
FWIW, I need a longer timeout on my local Linux machine for the landing page screenshots and I still get a minor offset for one of the figures.
Ya I find the darwin ones helpful for local development so I can see if something is wrong (or fixed) without having to push it and wait a few minutes.
Totally fine for me to add the timeout if that fixes it – or does that difference still appear?
Sure, makes sense with the Darwin screenshots!
The last diff image was even with a long timeout. I can submit a PR later if I figure something out, on this machine I got a warning that the OS is not supported officially by Playwright.