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

rework page preview to use transformers. page frame now uses placeholders #24

Merged
merged 11 commits into from
Mar 1, 2024

Conversation

ylebre
Copy link
Contributor

@ylebre ylebre commented Feb 19, 2024

No description provided.

@ylebre ylebre requested a review from poef February 19, 2024 13:51
@poef
Copy link
Member

poef commented Feb 21, 2024

Ik zie dat de placeholders formaat '{placeholder}' hebben. Moustache gebruikt '{{placeholder}}', en javascript templated strings gebruiken '${placeholder}'. Ik zou liever een van die 2 zien, in de rest van de code vallen ze meer op.

@ylebre
Copy link
Contributor Author

ylebre commented Feb 23, 2024

I've opted to use {{ }}. The syntax for javascript templated strings might raise the expectation of the placeholders being able to do more than just be a simple placeholder to be replaced.

@poef
Copy link
Member

poef commented Feb 27, 2024

I've opted to use {{ }}. The syntax for javascript templated strings might raise the expectation of the placeholders being able to do more than just be a simple placeholder to be replaced.

Is there a reason to match (\s+){{...}} instead of just {{...}} ? It feels a bit arbitrary.

@ylebre
Copy link
Contributor Author

ylebre commented Mar 1, 2024

Yes - it will take the space-symbols from the start of the line to do indenting with, so the generated code will be indented properly as well.

@poef poef merged commit 045936d into main Mar 1, 2024
1 check passed
@poef poef deleted the feature/page-frame-rebuild3 branch March 1, 2024 14:45
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