Skip to content

Implement page variable functionality#627

Merged
yamgent merged 9 commits into
MarkBind:masterfrom
jamos-tay:page-local-variables
Feb 4, 2019
Merged

Implement page variable functionality#627
yamgent merged 9 commits into
MarkBind:masterfrom
jamos-tay:page-local-variables

Conversation

@jamos-tay

@jamos-tay jamos-tay commented Jan 23, 2019

Copy link
Copy Markdown
Contributor

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [X] Enhancement to an existing feature

Fixes #575

What is the rationale for this request?

Extension of included variable functionality, allow users to specify local variables within the same page.

What changes did you make? (Give an overview)

Users can specify local variables with the variable key word:

<variable name="color">red</variable>

They can use it within the page like normal variables.

Is there anything you'd like reviewers to focus on?

Should variables cascade into includes? I'm thinking no, it can be confusing if the user is trying to trace where a variable came from and can't find it. If the user wishes for them to be used they can include it specifically:

<variable name="x">5</variable>
<include ... >
  <span id="x">{{ x }}</span>
</include>

All page variables are processed before any rendering is done, so users cannot change the value of variables mid page:

<variable name="x">5</variable>
{{ x }} // 6
<variable name="x">6</variable>
{{ x }} // 6

I don't think supporting this is a good idea since we won't be able to use nunjucks and would have to write our own parser. Users can use {% set %} as an alternative. Currently a warning is shown if the user tries to reassign a variable.

@damithc

damithc commented Jan 23, 2019

Copy link
Copy Markdown
Contributor

All page variables are processed before any rendering is done, so users cannot change the value of variables mid page:

should we call them constants instead?

<const name="MAX_SIZE">5</const>

@jamos-tay

jamos-tay commented Jan 23, 2019

Copy link
Copy Markdown
Contributor Author

We could, but aren't the global variables kind of like constants too, in that case?

@damithc

damithc commented Jan 24, 2019

Copy link
Copy Markdown
Contributor

We could, but aren't the global variables kind of like constants too, in that case?

At the moment, they are too.

@yamgent

yamgent commented Jan 24, 2019

Copy link
Copy Markdown
Member

should we call them constants instead?

<const name="MAX_SIZE">5</const>

But if the users can do a {% set %} on them, then it is confusing to call them constants? :P

@acjh

acjh commented Jan 24, 2019

Copy link
Copy Markdown
Contributor

We may also consider implementing overriding the sub-sites' variables in future.

@damithc

damithc commented Jan 24, 2019

Copy link
Copy Markdown
Contributor

But if the users can do a {% set %} on them, then it is confusing to call them constants? :P

That's true.

We have several types of variables each behaving in a different way; we should come up with a good terminology to differentiate them.

@jamos-tay

Copy link
Copy Markdown
Contributor Author

Sure - maybe we should just leave it as variable for now then since that's how variables.md is named?

@jamos-tay jamos-tay force-pushed the page-local-variables branch from ff22bd5 to c724d83 Compare January 28, 2019 06:31
@jamos-tay jamos-tay changed the title [WIP] Implement page variable functionality Implement page variable functionality Jan 28, 2019
@jamos-tay

Copy link
Copy Markdown
Contributor Author

Implemented page variables + included variables, should be ready

Comment thread test/test_site/_markbind/variables.md Outdated
Comment thread docs/userGuide/reusingContents.md Outdated
Comment thread docs/userGuide/reusingContents.md Outdated
Comment thread test/test_site/testPageVariables.md Outdated
@jamos-tay

Copy link
Copy Markdown
Contributor Author

Updated, page variables can now cascade into included files.

Page variables are now treated exactly like included variables, global > outer > inner priority etc.. The only difference is how they're defined (span vs variable)

Comment thread test/test_site/testPageVariablesInInclude.md Outdated
Comment thread docs/userGuide/reusingContents.md Outdated
@jamos-tay jamos-tay force-pushed the page-local-variables branch from 0c63d84 to eafb330 Compare January 31, 2019 13:15
Update documentation
@jamos-tay

Copy link
Copy Markdown
Contributor Author

Updated

@yamgent yamgent added this to the v1.17.2 milestone Feb 2, 2019
@yamgent yamgent merged commit 6e2ea26 into MarkBind:master Feb 4, 2019
@damithc

damithc commented Feb 4, 2019

Copy link
Copy Markdown
Contributor

Good work on this feature @jamos-tay (and @yamgent, @acjh). I've started using it and worked great so far. What do you guys think of allowing the reuse context to access page variables using the # operator?

e.g., <include src="foo.md#title"/> (where title is a page variable in foo.md)

@acjh

acjh commented Feb 4, 2019

Copy link
Copy Markdown
Contributor

Isn't that the syntax for fragments?

@damithc

damithc commented Feb 4, 2019

Copy link
Copy Markdown
Contributor

Isn't that the syntax for fragments?

Yes. a variable is like a hidden fragment, right? Of course name clashes is a concern but in that case we can follow the same rules as multiple segments with same ID?

@jamos-tay

Copy link
Copy Markdown
Contributor Author

Hmm... I think that implementation might have a few unwanted implications.

For example, if the reasoning is <include src="foo.md#title"/> renders <variable name="title"> fragment in the parent page and therefore title should apply to the parent page, then wouldn't <include src="foo.md"/> render all the variables in foo.md in the parent page and thus all inner variables should be applied? Which seems to be a case of an inner page modifying an outer page's content, which might go against what we want.

@damithc

damithc commented Feb 4, 2019

Copy link
Copy Markdown
Contributor

For example, if the reasoning is <include src="foo.md#title"/> renders <variable name="title"> fragment in the parent page and therefore title should apply to the parent page, then wouldn't <include src="foo.md"/> render all the variables in foo.md in the parent page and thus all inner variables should be applied? Which seems to be a case of an inner page modifying an outer page's content, which might go against what we want.

Accessing a variable value from the parent doesn't mean the variable is raised to the level of the parent. It's simply reading the value only.

I was hoping to use variables as properties of the page. e.g., title, summary, level, difficulty etc. Similar to a public varaible of a Java object, it makes sense to be able to access properties of the object from outside.

Of course, I can use <span id="title" class="d-none">abc</span> instead. Using variables is slightly more convenient.

@jamos-tay

Copy link
Copy Markdown
Contributor Author

Hmm okay, that seems reasonable

It would have to be treated separately from a regular include, though

@damithc

damithc commented Feb 8, 2019

Copy link
Copy Markdown
Contributor

Moved the discussion to #682

@ang-zeyu ang-zeyu mentioned this pull request Sep 13, 2020
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.

Allow declaration of variable values in the same source file

4 participants