Repository navigation
Refactored to use twig from library folder - #1
Conversation
cybtachyon
left a comment
cybtachyon
left a comment
There was a problem hiding this comment.
Just a couple comments. Looks great!
| // @todo Move to own JS file & Drupal Settings config var. | ||
| $markup = <<<HTML | ||
| <div id="editor_holder"></div> | ||
| <div id="magic-pixie-dust"></div> |
There was a problem hiding this comment.
Editor enhancements look good - but magic-pixie-dust isn't that descriptive as to what's happening here. Maybe magic-schema-container?
| static $metadata; | ||
|
|
||
| // Use static pattern to avoid rebuilding multiple times per request. | ||
| if (is_null($metadata)) { |
There was a problem hiding this comment.
Might be cleaner to replace this with the check-and-return anti-pattern, but may just be my personal preference.
if (!is_null($metadata)) {
return $metadata;
}
$it = new Recurs...
| // Create the pattern from defaults. | ||
| $pattern = $this->createPattern( | ||
| (object) [ | ||
| '$schema' => 'http =>//json-schema.org/draft-04/schema#', |
There was a problem hiding this comment.
Should these defaults be in the PatternkitPattern class?
There was a problem hiding this comment.
That whole section needs a refactor. The problem was that some folders have a .json, but no .twig, and some vice versa. This is particularly problematic because of the nested dependencies. no_html.twig was a recurring one in particular.
There was a problem hiding this comment.
Gotcha. I'll put it into the roadmap, thanks!
| * Twig engine instance object. | ||
| */ | ||
| public function getTwigInstance() { | ||
| static $twig_engine; |
There was a problem hiding this comment.
Should this be determinate on if tfd7 is already loaded or not? I'm wondering about the overhead of potentially bootstrapping the twig engine twice, although we probably need to test if it actually makes an impact or not.
There was a problem hiding this comment.
I replaced tfd7 entirely, but now that I think of it, you guys probably use it for other stuff. TFD7 does a /lot/ of extra work here.
Invoking twig with the namespacing requirements means you're paying for the class invocation regardless of approach, so at the very least this work is siloed from that code.
|
|
||
| // Namespacing is not necessary here, as each "engine" is unique to | ||
| // the "library". | ||
| $loader->addPath($templatesDirectory); |
There was a problem hiding this comment.
Haven't tested yet, but does this preclude support twig libraries that depend on namespaces?
Will update review comment if I get a chance to test it out before you get to this.
There was a problem hiding this comment.
no, you can add a namespace here. I just didn't because then I'd have to overload the FileLoader. I actually started to do that and then realized that I didn't need them.
There was a problem hiding this comment.
overload -> subclass
| if ($theme->engine !== 'twig' || !isset($theme->info['namespaces'])) { | ||
| continue; | ||
| // If undefined, create the libraries metadata. | ||
| if (is_null($libraries)) { |
There was a problem hiding this comment.
Let's use if ($libraries === null) { here.
There was a problem hiding this comment.
where are the coding standards you are following?
There was a problem hiding this comment.
https://mojo.redhat.com/docs/DOC-1133285 is_null is typically an unncessary function call, although I think PHP5.4+ inlines it before running
| <div id="editor_holder"></div> | ||
| <div id="magic-pixie-dust"></div> | ||
| <script type="text/javascript"> | ||
| let target = document.getElementById("magic-pixie-dust"); |
There was a problem hiding this comment.
Are we risking compatibility issues by using let here and not using a transpiler?
| */ | ||
| public function renderTwigTemplate($template, array $variables = array()) { | ||
|
|
||
| $content = ''; |
There was a problem hiding this comment.
Unnecessary variable declaration - remove if convenient.
There was a problem hiding this comment.
It's not unnecessary in the exception short circuit.
There was a problem hiding this comment.
scratch that, it looks like I fixed that at some point.
| * Implements hook_patternkit_library(). | ||
| */ | ||
| function patternkit_patternkit_library() { | ||
| // @todo Replace with a Service that calls a Factory. |
There was a problem hiding this comment.
Should we be removing support for themes to add pattern libraries? Maybe make it an option?
My reasoning is in Drupal 8, adding the 'namespace' key to the theme is the generally accepted way to add component libraries via the Component Libraries module.
There was a problem hiding this comment.
If it sounds good to you, I'll get a patch going for the following:
- Populate libraries from both hook usage as well as theme namespaces
- Set the default 'enabled' state of each library based on module/theme 'enabled' state
- Allow overriding the 'enabled' state of each library in the admin UI / via variables
There was a problem hiding this comment.
I replaced the namespace discovery stuff. no reason not to allow both.
| <?php | ||
| /** | ||
| * @file | ||
| * customer-portal-kbase-copy |
There was a problem hiding this comment.
What's this annotation here?
| // Create the pattern from defaults. | ||
| $pattern = $this->createPattern( | ||
| (object) [ | ||
| '$schema' => 'http =>//json-schema.org/draft-04/schema#', |
There was a problem hiding this comment.
Gotcha. I'll put it into the roadmap, thanks!
|
Looks good to me! Thanks again; ready to merge. |
2 participants
You'll need to put Twig in the /sites/all/libraries/Twig folder
I separated the hook_patternkit_library out into a custom module, where I also supply the actual components.
Happy to do a demo with you.