Refactored to use twig from library folder #1

SiliconValet · 2018-02-02T02:06:00Z

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.

cybtachyon

Just a couple comments. Looks great!

cybtachyon · 2018-02-02T16:57:17Z

src/PatternkitDrupalTwigLib.php

@@ -86,8 +86,13 @@ public function getEditor($subtype = NULL,
      : $config;
    // @todo Move to own JS file & Drupal Settings config var.
    $markup = <<<HTML
-<div id="editor_holder"></div>
+<div id="magic-pixie-dust"></div>


Editor enhancements look good - but magic-pixie-dust isn't that descriptive as to what's happening here. Maybe magic-schema-container?

cybtachyon · 2018-02-02T17:01:29Z

src/PatternkitDrupalTwigLib.php

+    static $metadata;
+
+    // Use static pattern to avoid rebuilding multiple times per request.
+    if (is_null($metadata)) {


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...

cybtachyon · 2018-02-02T17:03:40Z

src/PatternkitDrupalTwigLib.php

+          // Create the pattern from defaults.
+          $pattern = $this->createPattern(
+            (object) [
+              '$schema'    => 'http =>//json-schema.org/draft-04/schema#',


Should these defaults be in the PatternkitPattern class?

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.

Gotcha. I'll put it into the roadmap, thanks!

cybtachyon · 2018-02-02T17:06:29Z

src/PatternkitDrupalTwigLib.php

+   *   Twig engine instance object.
+   */
+  public function getTwigInstance() {
+    static $twig_engine;


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.

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.

cybtachyon · 2018-02-02T17:08:01Z

src/PatternkitDrupalTwigLib.php

+
+          // Namespacing is not necessary here, as each "engine" is unique to
+          // the "library".
+          $loader->addPath($templatesDirectory);


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.

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.

overload -> subclass

cybtachyon · 2018-02-02T17:12:17Z

patternkit.module

-    if ($theme->engine !== 'twig' || !isset($theme->info['namespaces'])) {
-      continue;
+  // If undefined, create the libraries metadata.
+  if (is_null($libraries)) {


Let's use if ($libraries === null) { here.

where are the coding standards you are following?

https://mojo.redhat.com/docs/DOC-1133285 is_null is typically an unncessary function call, although I think PHP5.4+ inlines it before running

cybtachyon · 2018-02-02T17:24:19Z

src/PatternkitDrupalTwigLib.php

 <script type="text/javascript">
+  let target = document.getElementById("magic-pixie-dust");


Are we risking compatibility issues by using let here and not using a transpiler?

cybtachyon · 2018-02-02T17:33:30Z

src/PatternkitDrupalTwigLib.php

+   */
+  public function renderTwigTemplate($template, array $variables = array()) {
+
+    $content = '';


Unnecessary variable declaration - remove if convenient.

It's not unnecessary in the exception short circuit.

scratch that, it looks like I fixed that at some point.

cybtachyon · 2018-02-06T15:11:50Z

patternkit.module

 }

 /**
 * Implements hook_patternkit_library().
 */
 function patternkit_patternkit_library() {
-  // @todo Replace with a Service that calls a Factory.


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.

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

I replaced the namespace discovery stuff. no reason not to allow both.

cybtachyon · 2018-02-06T15:18:01Z

src/PatternkitTwigWrapper.php

+<?php
+/**
+ * @file
+ * customer-portal-kbase-copy


What's this annotation here?

cybtachyon · 2018-02-06T15:18:56Z

src/PatternkitDrupalTwigLib.php

+          // Create the pattern from defaults.
+          $pattern = $this->createPattern(
+            (object) [
+              '$schema'    => 'http =>//json-schema.org/draft-04/schema#',


Gotcha. I'll put it into the roadmap, thanks!

cybtachyon · 2018-02-06T15:41:34Z

Looks good to me! Thanks again; ready to merge.

Jason Smith added 4 commits February 1, 2018 21:03

Refactored to use twig from library folder

fc4ca6e

More documentation and cleanup

043cb79

Used shadowdom and magic to make editor easier

42c4810

Fix the issue in jsoneditor

2f3758e

cybtachyon requested changes Feb 2, 2018

View reviewed changes

Jason Smith added 2 commits February 2, 2018 12:53

Changed name of target to something boring

b120ccf

Support multiple namespaces.

142879b

cybtachyon requested changes Feb 6, 2018

View reviewed changes

Support the theme namespace discovery

a6bfc9c

cybtachyon merged commit cc9a120 into master Feb 6, 2018

cybtachyon deleted the feature/jassmith/RefactorTwigLib branch February 6, 2018 15:41

		<script type="text/javascript">
		let target = document.getElementById("magic-pixie-dust");

Uh oh!

Refactored to use twig from library folder #1

Refactored to use twig from library folder #1

Uh oh!

Conversation

SiliconValet commented Feb 2, 2018

Uh oh!

cybtachyon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cybtachyon commented Feb 6, 2018

Uh oh!

Uh oh!