-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
[DRAFT] Example of what the course would look like with the new renderer #1216
base: main
Are you sure you want to change the base?
Conversation
src/components/language_picker.html
Outdated
@@ -0,0 +1,39 @@ | |||
{% import "Macros" as macros %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Macros work (import the name defined in the book.toml
and not the file name. You could name it with the file name too)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see! This is the difference in thinking: I was expecting the user to declare an include directory and then rely solely on the mechanisms in the template engine (import a file by it's relative path, for example).
I believe you've instead built more infrastructure to make it easier to use — but the problem with that is that the user now has to understand the custom logic here (and you and I have to document and maintain this).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to support both options, but to do use a macro like this in index.hbs
, you will need to write sometimes 4 {
in a row {{{{ attribute | get(key="style") }}}}
, I find that very clunky, when using HTML syntax is pretty easy to understand for people already writing HTML.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might even have to write 6 in a row when I think about it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to support both options
Please don't: avoid building up your own vocabulary here since nobody but you will know about it 🙂 I would like us to have as little code to maintain as possible here.
src/components/language_picker.html
Outdated
<button id="language-toggle{{ counter }}" class="icon-button" type="button" | ||
title="Change language" aria-label="Change language" | ||
aria-haspopup="true" aria-expanded="false" | ||
aria-controls="language-list{{ counter }}" style="{{ attributes | get(key="style", default="") }}"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading attributes passed in HTML
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should go away in a MVP — I would like to keep this super simple at first.
src/components/language_picker.html
Outdated
{% for identifier, language_name in get_context(key="output.i18n.languages") %} | ||
<li role="none"> | ||
<a id="{{ identifier }}" | ||
href="{{ macros::get_rendered_path(identifier=identifier) }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling the macro
src/components/language_picker.html
Outdated
href="{{ macros::get_rendered_path(identifier=identifier) }}" | ||
style="color: inherit;"> | ||
<button role="menuitem" class="theme {% if identifier == language %} theme-selected {% endif %}"> | ||
{% include "Included" %} {{ language_name }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Include directive
@@ -0,0 +1,3 @@ | |||
{% macro get_rendered_path(identifier) %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Defining macro
theme/index.hbs
Outdated
} | ||
</script> | ||
|
||
<LanguagePicker style="color: red;"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<LanguagePicker />
syntax is not supported
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now tera
syntax is not supported in index.hbs
but I will add it later, it is going to look a bit ugly though.
src/components/language_picker.html
Outdated
<script> | ||
let langToggle = document.getElementById("language-toggle{{ counter }}"); | ||
let langList = document.getElementById("language-list{{ counter }}"); | ||
{% raw %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a raw block for the Javascript block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, but then I think the {{
below has a {
too many?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's a mistake actually (from copy pasting the HBS one), but still valid JS
Co-authored-by: Martin Geisler <[email protected]>
No description provided.