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

Refactor html_shell.rs to improve readability #109

Merged
merged 3 commits into from
Jan 13, 2022
Merged

Refactor html_shell.rs to improve readability #109

merged 3 commits into from
Jan 13, 2022

Conversation

Asha20
Copy link
Contributor

@Asha20 Asha20 commented Jan 11, 2022

I've added some tests to html_shell.rs. They don't really assert anything, but I used them to be able to quickly print out the output while refactoring to make sure that it looks right. I figured I'd leave them in for now in case you wanted to try running the code yourself while reviewing.

window.__PERSEUS_RENDER_CFG now gets defined at the top of <head> instead of bottom, but I don't think that should change much. You know the code better than me though, so I figured I'd mention it just in case.

I tried running the tests on my laptop, but they took a really long time since my specs aren't that great and I also ran into an issue with geckodriver. Maybe you could run the tests in CI?

Let me know if you think there's more room for improvement or something should be changed!

Closes #108

@arctic-hen7
Copy link
Member

arctic-hen7 commented Jan 11, 2022

Great, thanks! I'll have a look at this in depth very soon. As long as the render configuration isn't defined on the other side of the interpolation boundary, it should be fine.

Yes, geckodriver is frequently the bane of my existence. If you get errors about the session already being started, you just have to re-run the failing tests.

Copy link
Member

@arctic-hen7 arctic-hen7 left a comment

Choose a reason for hiding this comment

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

This is great, thanks! I love the architecture, it's much more intuitive!

@arctic-hen7
Copy link
Member

In future, could you sign your commits please? No worries for this and #110 though.

@arctic-hen7 arctic-hen7 merged commit 69e9f72 into framesurge:main Jan 13, 2022
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.

Refactoring html_shell.rs to improve readability
2 participants