Skip to content
This repository has been archived by the owner on Dec 10, 2019. It is now read-only.

Parametered partials hogan #4

Closed
wants to merge 15 commits into from
Closed

Parametered partials hogan #4

wants to merge 15 commits into from

Conversation

e2tha-e
Copy link
Contributor

@e2tha-e e2tha-e commented Jul 28, 2016

Addresses pattern-lab/patternlab-node#250

Summary of changes:

  • Replace Mustache with Hogan for performance benefits
  • patternMatcher must work with .extendedTemplate and not .template for recursive parametered partials to work
  • new functions to register and preprocess partials
  • paramToJson copied from main patternlab-node repo - its purpose it specific to pl's Mustache syntax

},
"devDependencies": {
"hogan.js": "^3.0.2",
"json5": "^0.5.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't these two be dependencies rather than devDependencies?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah! They are. The diff misled me. :)

@geoffp
Copy link
Contributor

geoffp commented Aug 3, 2016

Good catch on using .extendedTemplate.

Would you please:

  1. Provide an example of the syntax that all this added machinery is intended to support, and
  2. Give me rundown of the new partial registration process and why it's needed

@e2tha-e
Copy link
Contributor Author

e2tha-e commented Aug 4, 2016

@geoffp
I've added JSDocs to try and explain item 2.
As for item 1:

@e2tha-e
Copy link
Contributor Author

e2tha-e commented Aug 4, 2016

FYI, neither Mustache nor Hogan has a native registerPartial method. Handlebars, and probably many other "more powerful" templating languages do have this method. I'm just applying an already existing convention to Hogan.

@geoffp
Copy link
Contributor

geoffp commented Aug 5, 2016

I'm trying to read the big PR that you linked to, but I only have so much time in a day for code review and I just can't get through it all in one sitting or even two. I also still don't have an example of the syntax you're trying to support.

Having worked extensively on pattern parameters and style modifiers, how possible do you think it would be to move most of the machinery of dealing with them into the Mustache engine itself? They're not wanted or needed for other engines, and it adds a huge amount of complexity to the main app.

@e2tha-e
Copy link
Contributor Author

e2tha-e commented Aug 5, 2016

In the links I provided, I highlighted the lines where registerPartial and preprocessPartials are called. Here they are out of context:

pattern.registerPartial(patternlab);
engine.preprocessPartials(pattern_assembler, patternlab);

Maybe I'm not fully understanding what sort of example or explanation you want. Please do ask for more specifics if you want.

I'll definitely give the proposal to make my changes Mustache-specific some thought. But in actuality, I think my changes are simplifying the main app, especially in regards to partial inclusion. If you look at the code statistics for pattern-lab/patternlab-node/pull/416 , I reduced the amount of code by 72 lines. I'm obviously more than compensating by increasing the amount of code in patternengine-node-mustache, but that's Mustache-specific. I tested my changes to the main app with the Handlebars engine, and it worked perfectly, with just a minor pr to patternengine-node-handlebars.

@bmuenzenmeyer
Copy link
Member

Please see pattern-lab/patternlab-node#603 for an explanation of why I am closing this PR.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants