-
Notifications
You must be signed in to change notification settings - Fork 124
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
Clean up docs & tests for variables #2301
Conversation
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.
Added some comments for ease of reviewing
@@ -36,10 +36,10 @@ The default output format is `"ddd D MMM"` e.g. Fri 6 Mar | |||
{{ njcode('base1 | date(format2, 0)') }} :glyphicon-arrow-right: {{ base1 | date(format2, 0) }}<br/> | |||
{{ njcode('base1 | date(format2, 10)') }} :glyphicon-arrow-right: {{ base1 | date(format2, 10) }}<br/> | |||
|
|||
#### Page variables | |||
Dates can be supplied using [page variables](../reusingContents.html#variables) for convenience. |
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.
Note: page variables no longer supported via <variable>
syntax, see #2283
@@ -9,6 +9,5 @@ | |||
<variable name="global_variable_overriding_included_variable">Global Variable Overriding Included Variable</variable> | |||
<variable name="global_variable">Global Variable</variable> | |||
<variable name="page_global_variable_overriding_page_variable">Global Variable Overriding Page Variable</variable> | |||
<variable from="variable.json"></variable> |
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.
Note: removed as this way of importing does not work in variables.md
@@ -71,6 +71,10 @@ Page Variable with {{ global_variable }} | |||
|
|||
**Test Page Variable and Included Variable Integrations** | |||
|
|||
{% set outerNunjucksVariable %} |
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.
Note: adding this because in testPageVariablesInInclude.md
below we will test if this outer variable is leaking into the inner page.
@@ -1,3 +1,3 @@ | |||
**Outer Page Variable Should Not Leak Into Inner Pages** | |||
|
|||
{{ nested_page_variable or "Outer Page Variable Should Not Leak Into Inner Pages" }} | |||
{{ outerNunjucksVariable or "Outer Page Variable Should Not Leak Into Inner Pages" }} |
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.
Note: updating this because it appears to be outdated. we used to have nested_page_variable
defined in index.md
but that has been deleted for a long time. See yucheng11122017@2ddf00e
@@ -0,0 +1,4 @@ | |||
<variable name="example"> | |||
To inject this HTML segment in your MarkBind files, use {{ example }} where you want to place 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.
Note: for some reason there's no variables.md
in this and the other site above, but we should include it to avoid unnecessary warning.
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.
LGTM
What is the purpose of this pull request?
Overview of changes:
Fixes #2283
variables.md
filevariables.md
file to remove unnecessary warning about missing file during functional testsAnything you'd like to highlight/discuss:
Was expecting a need to update functional tests but doesn't seem to be required due to how
variables.md
and how nunjucks is processed before generating the HTML.Testing instructions:
markbind init
does not contain warning of "variable has no name"npm run test
no longer shows any warning about "no such file" for ".../variables.md" or the variable has no name warningProposed commit message: (wrap lines at 72 characters)
Clean up docs & tests for variables
Our docs on page variable is outdated due to feature removal. The
incorrect use of variable imports from json files within our variables.md
is problematic as it is no longer supported. A outdated test case still
exists for testing of page variable override. There are two warnings
of missing variables.md files in our functional test suite.
Keeping docs, test cases and implementation up-to-date is important.
Let's fix the docs, update the test cases, remove unnecessary warning
and ensure all variables.md do not contain the imports.
The above will fix the related issues around page variables and
variable imports.
Checklist: ☑️
Breaking change description:
A newly generated site (e.g. via
markbind init
) will no longer contain the following erroneous line in_markbind/variables.md
:This way of importing variables from an external file within the global variables.md is not supported.
For existing sites migrating to the latest version, please check that you do not have the above line in your
_markbind/variables.md
file as it will not work as expected and will log a warning. Please remove such a line if found.