Skip to content

feat(templatesConfig): helpers and options transferred to Template #136

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

Merged
merged 2 commits into from
Sep 29, 2015

Conversation

Jerska
Copy link
Member

@Jerska Jerska commented Sep 28, 2015

The main goal of this PR is to let the Template component handle the logic related to itself instead of its parents.

New parameter passed to the init and render method: templatesConfig.
It replaces templateHelpers and is defined in Instantsearch.js:

this.templatesConfig = {
  helpers: {
    // ...
  },
  options: {}
}

To define templating options, you only need to use:

search.templatesConfig.options.delimiters = '[[ ]]';

or

search.templatesConfig.options = {
  delimiters: '[[ ]]',
  sectionTags: [{ o: '_foo', c: 'foo' }]
}

This parameter embeds both the helpers and the Hogan options.
It uses the same chain that the templateHelpers previously used to be passed from a widget to the Template component.

The Template component now also accepts a templateDefault parameter which allows us to ignore the templates options on Hogan.compile calls if there is no template provided.

The template prop is not required anymore down the chain, but renderTemplate will throw an error if both the default template and the passed templated are null or undefined (but in case we want to display nothing, the empty string works).

Closes #99.

Alternatives:

  • Exposing the template renderer as a parameter of the template function:
var mytemplate = '....';
//...
template: function (data, hogan) {
  Hogan.compile(mytemplate, { delimiters: '[[ ]]' }).render(data);
}
  • Using the undocumented context feature of React to avoid passing the config down the chain, and only declare it as a context parameter on the first render call.

@Jerska Jerska force-pushed the feature/templatesConfig branch 2 times, most recently from ae02d82 to f561d08 Compare September 28, 2015 10:01
@vvo
Copy link
Contributor

vvo commented Sep 28, 2015

this.templatesConfig = {
helpers: {
// ...
},
config: {}
}

I believe it's options:{}

Also we should use hoganCompileOptions to be specific about what's inside here. https://github.com/twitter/hogan.js#compilation-options

If you agree, update :D

@vvo
Copy link
Contributor

vvo commented Sep 28, 2015

var mytemplate = '....';
//...
template: function (data) {
Hogan.compile(mytemplate, { delimiters: '[[ ]]' }).render(data);
}

Do you mean template: function (data, hogan) {}?

@Jerska
Copy link
Member Author

Jerska commented Sep 28, 2015

@vvo Fixed both.
About hoganCompileOptions, I strongly disagree since here, this is fairly template engine agnostic, the only part of instantsearch.js which knows we're using Hogan is the renderTemplate function, and I'm pretty sure that if we decided to switch to another templating engine, this options parameter would be useful.

var content;

if (template === null || typeof template === 'undefined') {
Copy link
Contributor

Choose a reason for hiding this comment

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

typeof template === 'undefined'

is only needed when the variable may not have been defined in the first place. Which is not the case here because it is a parameter of the function. I would suggest that instead :

template === undefined

Copy link
Contributor

Choose a reason for hiding this comment

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

I get the edge case here (people redefining undefined global var). But those websites have other issues that we do not want to fix here.

@vvo
Copy link
Contributor

vvo commented Sep 28, 2015

this is fairly template engine agnostic

But we are not template engine agnostic. We use hogan and thus should not hide it (and we do not right now). I think there are little chances that we switch to another template engine. And if we do then it would be a major breakage otherwise it means there's no added value to the change.

It will be easier to grasp for people what are theses options and to what they map (the hogan documentation).

The only place where we would benefit from another templating engine would be to use something more featureful than hogan and such we would break compat.

@@ -2,6 +2,7 @@
<label>
<input type="checkbox" {{#isRefined}}checked{{/isRefined}} />
{{label}}
{{_refined}}!{{/refined}}
Copy link
Contributor

Choose a reason for hiding this comment

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

where do _refined comes from and is it different from "isRefined"?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was just to illustrate that the options worked, and I didn't want to do it with the delimiters options, because it would have meant changing all delimiters in example/templates/*. It's no different from isRefined, the only difference is that it's using the section tags of Hogan (you can see there is no # in the opening tag).
We can remove it.

@vvo
Copy link
Contributor

vvo commented Sep 28, 2015

I am worried about the "defaultTemplate" being passed everywhere.

Because it adds some good code to solve the "For some widgets I want to use my options, for others I want to use my options" usecase.

I do not have a solution for this thought right now.

@Jerska
Copy link
Member Author

Jerska commented Sep 28, 2015

Îf we really want to state that we're using Hogan, I'd rather replace templatesConfig by hoganConfig than options by hoganCompileOptions
Maybe

hoganConfig = {
  helpers,
  compileOptions
}

@Jerska
Copy link
Member Author

Jerska commented Sep 28, 2015

About the defaultTemplate, it's not only used for the options, it also moved the _.defaults(templates, defaultTemplates) logic in the widgets to the Template component, while removing the lodash dependency to defaults. But I understand that you're not completely happy with it.

New parameter passed to the `init` and `render` method: `templatesConfig`.
It replaces `templateHelpers`.

This parameter embeds both the `helpers` and the `Hogan` options.
It uses the same chain that the `templateHelpers` previously used to
be passed from a widget to the `Template` component.

The `Template` component now also accepts a `templateDefault` parameter
which allows us to ignore `Hogan`'s `options` if there is no
`template` provided

Closes #99.
@vvo vvo force-pushed the feature/templatesConfig branch from f561d08 to 456d781 Compare September 28, 2015 11:54
@vvo vvo force-pushed the feature/templatesConfig branch 2 times, most recently from 8d433c0 to 40fe8ad Compare September 28, 2015 17:32
+ using a shared behavior to handle all template* instantsearch
instance
config dependencies
+ renamed templatesConfig.options to templatesConfig.compileOptions
+ added `templates` (header, body, footer) to `stats` BREAKING CHANGE
+ jsDoc for `stats`
@vvo vvo force-pushed the feature/templatesConfig branch from 40fe8ad to 3c28aac Compare September 29, 2015 08:35
vvo pushed a commit that referenced this pull request Sep 29, 2015
feat(templatesConfig): helpers and options transferred to Template
@vvo vvo merged commit 35825df into develop Sep 29, 2015
@vvo vvo deleted the feature/templatesConfig branch September 29, 2015 08:48
@vvo vvo removed the in progress label Sep 29, 2015
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.

3 participants