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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Twig PHP Engine in Pattern Lab node #897

Open
wants to merge 22 commits into
base: dev
from

Conversation

Projects
None yet
8 participants
@EvanLovely
Member

EvanLovely commented Jul 17, 2018

馃帀 OK, here we go! This adds a Twig PHP Engine and implements it in the Twig Edition. This is really sweet because this will let us Unify Pattern Lab Core by bringing the most essential part of the PHP Pattern Lab world in: a real Twig environment using the actual engine.

I'll have to get into more later, but I've created @basalt/twig-renderer which is a JS module that takes config and gives back a render JS function that when called spins up an internal PHP server that exposes an API endpoint that renders Twig templates. It's all async and it can render 1,000 simple Twig templates in about 5s - and only spins up 1 PHP server for that @aleksip 馃槈

Take a peak @pattern-lab/devs and tell me what you think!

To test:

npm install
npm run bootstrap
cd packages/edition-twig
npm start

Bugs to fix:

  • Header & foote _meta patterns not rendering on individual pattern page (but do on view all pages)

EvanLovely and others added some commits Jun 29, 2018

Publish
 - @pattern-lab/cli@0.0.1-alpha.23
 - @pattern-lab/edition-node@1.0.0-alpha.13
 - @pattern-lab/edition-node-gulp@2.0.0-alpha.15

@bmuenzenmeyer bmuenzenmeyer changed the base branch from master to dev Jul 17, 2018

@aleksip

This comment has been minimized.

Member

aleksip commented Jul 17, 2018

Tried to install, but getting the same error as Travis:

npm ERR! enoent ENOENT: no such file or directory, rename '/home/travis/build/pattern-lab/patternlab-node/packages/engine-twig-php/node_modules/.staging/@basalt/twig-renderer-83c3d739/node_modules/@babel/code-frame' -> '/home/travis/build/pattern-lab/patternlab-node/packages/engine-twig-php/node_modules/.staging/@babel/code-frame-0e54f88a'

@EvanLovely

This comment has been minimized.

Member

EvanLovely commented Jul 17, 2018

馃 hmmm... @bmuenzenmeyer - any ideas on above error?

EvanLovely added some commits Jul 17, 2018

@bmuenzenmeyer

This comment has been minimized.

Member

bmuenzenmeyer commented Jul 17, 2018

@EvanLovely will take a look, first glance doesn't ring any bells for me

@coveralls

This comment has been minimized.

coveralls commented Jul 18, 2018

Coverage Status

Coverage increased (+1.2%) to 74.545% when pulling 5b0c56c on feature/engine-twig-php into 2b70ff4 on dev.

@bmuenzenmeyer

This comment has been minimized.

Member

bmuenzenmeyer commented Jul 18, 2018

Fixed this - originally fixed in 5ab3995 after some research into lock-file behavior with lerna bootstraps being unreliable. Since we pin all dependencies, we should still have deterministic installations

@bmuenzenmeyer bmuenzenmeyer self-requested a review Jul 18, 2018

@bmuenzenmeyer

馃帀 馃摙 鈿★笍 馃挘 馃挜
This is a pretty significant accomplishment in the history of Pattern Lab. Great work Evan! It's the culmination of so many discussions and technical hops.

.travis.yml Outdated
@@ -1,6 +1,7 @@
language: node_js
before_install:
- npm i -g npm

This comment has been minimized.

@bmuenzenmeyer

bmuenzenmeyer Jul 18, 2018

Member

I don't like doing this - as npm has been known to break itself even with minor or patch releases. Since the .travis.yml file should be supplying Node versions, it also reliably gets the npm version bundled with Node

I think this was added as a troubleshooting step. If so, please remove.

This comment has been minimized.

@sghoweri

sghoweri Jul 18, 2018

Contributor

Sorry, another PL Node newb question here: any particular reason why aren鈥檛 we using Yarn + Lerna Workspaces here?

This comment has been minimized.

@bmuenzenmeyer

bmuenzenmeyer Jul 18, 2018

Member

We focused on Lerna for our initial monorepo conversion. I don't want to get into the game of documenting everything with npm and yarn. npm 5+ has come a long way., blunting the original benefits of yarn to me. We do not use lockfiles (as you can see the problems in this PR) anyways, further blunting the need. I understand we could use yarn for dev and posture to users only npm, but I don't really understand the need for yet another tool. Please let me know if I am missing something here 馃槄

This comment has been minimized.

@hdwebpros

hdwebpros Jul 19, 2018

Less tools is better IMO :)

This comment has been minimized.

@EvanLovely

EvanLovely Jul 19, 2018

Member

Was done as a troubleshooting step; removed now.

@@ -3,7 +3,7 @@
"lerna": "3.0.0-beta.21"
},
"scripts": {
"bootstrap": "lerna bootstrap --hoist tap --hoist eslin* --hoist husky --hoist prettier --hoist pretty-quick",
"bootstrap": "lerna bootstrap --no-ci --hoist tap --hoist eslin* --hoist husky --hoist prettier --hoist pretty-quick",

This comment has been minimized.

@bmuenzenmeyer

bmuenzenmeyer Jul 18, 2018

Member

What does this do? I cannot find this flag on https://github.com/lerna/lerna

This comment has been minimized.

@sghoweri

sghoweri Jul 18, 2018

Contributor

Shouldn鈥檛 we move these to the lerna.json config in the root so these flags always get added?

This comment has been minimized.

@bmuenzenmeyer

bmuenzenmeyer Jul 18, 2018

Member

I'd be fine with moving it there in case someone has lerna installed globally and instead runs lerna bootstrap It sounds like you might have more experience with lerna, could you offer up a focused PR to do this @sghoweri ?

This comment has been minimized.

@sghoweri

sghoweri Jul 18, 2018

Contributor

Definitely - happy to help! 馃憤馃徏

This comment has been minimized.

@EvanLovely

EvanLovely Jul 19, 2018

Member

The --no-ci flag is part of v3, those docs are v2. https://github.com/lerna/lerna/tree/v3.0.0-beta.21#--ci

When I bumped npm from 5 to 6, lerna uses npm ci instead of npm install, and I had error messages surrounding it. Since I removed the Travis npm update, I also removed this tweak.

@@ -0,0 +1 @@
{"version":1,"timestamp":1531799832072,"graph":{"options":{"directed":true,"multigraph":false,"compound":false},"nodes":[{"v":"03-templates/01-homepage.twig","value":{"compileState":"clean"}},{"v":"03-templates/02-blog.twig","value":{"compileState":"clean"}},{"v":"03-templates/03-article-2col.twig","value":{"compileState":"clean"}},{"v":"03-templates/03-article.twig","value":{"compileState":"clean"}},{"v":"04-pages/00-homepage.twig","value":{"compileState":"clean"}},{"v":"04-pages/01-blog.twig","value":{"compileState":"clean"}},{"v":"04-pages/02-article.twig","value":{"compileState":"clean"}},{"v":"00-atoms/01-global/00-colors.twig","value":{"compileState":"clean"}},{"v":"00-atoms/01-global/01-fonts.twig","value":{"compileState":"clean"}},{"v":"00-atoms/01-global/02-animations.twig","value":{"compileState":"clean"}},{"v":"00-atoms/01-global/03-visibility.twig","value":{"compileState":"clean"}},{"v":"00-atoms/03-lists/00-unordered.twig","value":{"compileState":"clean"}},{"v":"00-atoms/03-lists/01-ordered.twig","value":{"compileState":"clean"}},{"v":"00-atoms/03-lists/02-definition.twig","value":{"compileState":"clean"}},{"v":"00-atoms/02-text/00-headings.twig","value":{"compileState":"clean"}},{"v":"00-atoms/02-text/02-blockquote.twig","value":{"compileState":"clean"}},{"v":"00-atoms/02-text/01-paragraph.twig","value":{"compileState":"clean"}},{"v":"00-atoms/02-text/03-inline-elements.twig","value":{"compileState":"clean"}},{"v":"00-atoms/02-text/04-time.twig","value":{"compileState":"clean"}},{"v":"00-atoms/02-text/05-preformatted-text.twig","value":{"compileState":"clean"}},{"v":"00-atoms/02-text/06-hr.twig","value":{"compileState":"clean"}},{"v":"00-atoms/04-images/00-logo.twig","value":{"compileState":"clean"}},{"v":"00-atoms/04-images/01-landscape-4x3.twig","value":{"compileState":"clean"}},{"v":"00-atoms/04-images/02-landscape-16x9.twig","value":{"compileState":"clean"}},{"v":"00-atoms/04-images/03-square.twig","value":{"compileState":"clean"}},{"v":"00-atoms/04-images/04-avatar.twig","value":{"compileState":"clean"}},{"v":"00-atoms/04-images/05-icons.twig","value":{"compileState":"clean"}},{"v":"00-atoms/04-images/06-loading-icon.twig","value":{"compileState":"clean"}},{"v":"00-atoms/04-images/07-favicon.twig","value":{"compileState":"clean"}},{"v":"00-atoms/05-forms/00-text-fields.twig","value":{"compileState":"clean"}},{"v":"00-atoms/05-forms/01-select-menu.twig","value":{"compileState":"clean"}},{"v":"00-atoms/05-forms/02-checkbox.twig","value":{"compileState":"clean"}},{"v":"00-atoms/05-forms/03-radio-buttons.twig","value":{"compileState":"clean"}},{"v":"00-atoms/05-forms/04-html5-inputs.twig","value":{"compileState":"clean"}},{"v":"00-atoms/06-buttons/00-buttons.twig","value":{"compileState":"clean"}},{"v":"00-atoms/07-tables/00-table.twig","value":{"compileState":"clean"}},{"v":"00-atoms/08-media/_00-video.twig","value":{"compileState":"clean"}},{"v":"00-atoms/08-media/_01-audio.twig","value":{"compileState":"clean"}},{"v":"01-molecules/00-text/00-byline.twig","value":{"compileState":"clean"}},{"v":"01-molecules/00-text/01-address.twig","value":{"compileState":"clean"}},{"v":"01-molecules/00-text/02-heading-group.twig","value":{"compileState":"clean"}},{"v":"01-molecules/00-text/03-blockquote-with-citation.twig","value":{"compileState":"clean"}},{"v":"01-molecules/00-text/04-intro-text.twig","value":{"compileState":"clean"}},{"v":"01-molecules/01-layout/01-two-up.twig","value":{"compileState":"clean"}},{"v":"01-molecules/01-layout/00-one-up.twig","value":{"compileState":"clean"}},{"v":"01-molecules/01-layout/02-three-up.twig","value":{"compileState":"clean"}},{"v":"01-molecules/01-layout/03-four-up.twig","value":{"compileState":"clean"}},{"v":"01-molecules/02-blocks/00-media-block.twig","value":{"compileState":"clean"}},{"v":"01-molecules/02-blocks/01-block-headline-byline.twig","value":{"compileState":"clean"}},{"v":"01-molecules/02-blocks/02-block-hero.twig","value":{"compileState":"clean"}},{"v":"01-molecules/02-blocks/03-block-thumb-headline.twig","value":{"compileState":"clean"}},{"v":"01-molecules/02-blocks/04-block-headline.twig","value":{"compileState":"clean"}},{"v":"01-molecules/02-blocks/05-block-inset.twig","value":{"compileState":"clean"}},{"v":"01-molecules/03-media/00-figure-with-caption.twig","value":{"compileState":"clean"}},{"v":"01-molecules/04-forms/01-comment-form.twig","value":{"compileState":"clean"}},{"v":"01-molecules/04-forms/00-search.twig","value":{"compileState":"clean"}},{"v":"01-molecules/04-forms/02-newsletter.twig","value":{"compileState":"clean"}},{"v":"01-molecules/05-navigation/01-footer-nav.twig","value":{"compileState":"clean"}},{"v":"01-molecules/05-navigation/00-primary-nav.twig","value":{"compileState":"clean"}},{"v":"01-molecules/05-navigation/02-breadcrumbs.twig","value":{"compileState":"clean"}},{"v":"01-molecules/05-navigation/03-pagination.twig","value":{"compileState":"clean"}},{"v":"01-molecules/05-navigation/04-tabs.twig","value":{"compileState":"clean"}},{"v":"01-molecules/06-components/00-social-share.twig","value":{"compileState":"clean"}},{"v":"01-molecules/06-components/01-accordion.twig","value":{"compileState":"clean"}},{"v":"01-molecules/06-components/02-single-comment.twig","value":{"compileState":"clean"}},{"v":"01-molecules/07-messaging/00-alert.twig","value":{"compileState":"clean"}},{"v":"02-organisms/00-global/00-header.twig","value":{"compileState":"clean"}},{"v":"02-organisms/00-global/01-footer.twig","value":{"compileState":"clean"}},{"v":"02-organisms/01-article/00-article-body.twig","value":{"compileState":"clean"}},{"v":"02-organisms/02-comments/00-comment-thread.twig","value":{"compileState":"clean"}},{"v":"02-organisms/03-sections/00-latest-posts.twig","value":{"compileState":"clean"}},{"v":"02-organisms/03-sections/01-recent-tweets.twig","value":{"compileState":"clean"}},{"v":"02-organisms/03-sections/02-related-posts.twig","value":{"compileState":"clean"}},{"v":"03-templates/00-layouts/00-site.twig","value":{"compileState":"clean"}},{"v":"03-templates/00-layouts/01-page-1col.twig","value":{"compileState":"clean"}},{"v":"03-templates/00-layouts/01-page-2col.twig","value":{"compileState":"clean"}},{"v":"04-pages/00-homepage~emergency.json","value":{"compileState":"clean"}}],"edges":[{"v":"04-pages/00-homepage~emergency.json","w":"04-pages/00-homepage.twig","value":{}}]}}

This comment has been minimized.

@bmuenzenmeyer

bmuenzenmeyer Jul 18, 2018

Member

This file is an artifact of sorts and should not be initially available as part of generation. It should not be ignored either,

This comment has been minimized.

@sghoweri

sghoweri Jul 18, 2018

Contributor

Why can鈥檛 this be gitgnored?... Directly committing auto generated assets = 馃槹

This comment has been minimized.

@bmuenzenmeyer

bmuenzenmeyer Jul 18, 2018

Member

Let's think through this outloud - perhaps it could be? or perhaps not? My current thinking is that teams would want to check this in along with changes - so that another dev pulls down the changes and can run PL with incremental builds on - allowing PL to only build what's changed. If this is gitignored, users pulling down changes will have incorrect dependencyGraphs upon build, leading to odd behavior. Perhaps we need more testing around how this works for teams.

This comment has been minimized.

@hdwebpros

hdwebpros Jul 19, 2018

I'm not following @bmuenzenmeyer 's logic. We work with usually 2 devs here doing Patternlab. 99% of the time we are editing patterns, assets, or corresponding json. If one of the devs pushes changes, we just pull and then start where they left off. I don't see how this autogenerated file would help.

My $0.02 -- If they push changes, this file would autogenerate regardless because I'd have to run PL regardless. I don't see a case for pushing this file so that someone could investigate this file and see the state that everything is in.

In fact, if someone really wanted to see this, they could edit their own .gitignore to allow it.

That's my $0.02, not sure if I'm helping or not here

This comment has been minimized.

@tburny

tburny Jul 19, 2018

As the person who implemented that feature:
This file caches the current build status of each pattern, so if either the data or the meta data changes after the last build, the file gets recompiled incrementally with all descendant patterns.

It can be regarded as a local build cache and thus should be added to .gitignore.

The alternative would be hell:
Version patterns, data and the generated output files with correct modification timestamps. This would lead to lots of potential merge conflicts in the dependency graph file when a few developers collaboratem, so
don't do this, ever.

This comment has been minimized.

@bmuenzenmeyer

bmuenzenmeyer Jul 19, 2018

Member

@hdwebpros

I don't see a case for pushing this file so that someone could investigate this file and see the state that everything is in.

Worth noting that no one would really look at this file manually.

@tburny

It can be regarded as a local build cache and thus should be added to .gitignore.

thanks for chiming in! ignore it is then

"@pattern-lab/engine-mustache": "^2.0.0-alpha.6",
"@pattern-lab/engine-twig-php": "^0.1.0",
"@pattern-lab/uikit-workshop": "^1.0.0-alpha.5",
"@pattern-lab/starterkit-twig-demo": "^4.0.0"

This comment has been minimized.

@bmuenzenmeyer

bmuenzenmeyer Jul 18, 2018

Member

Do you intend the Twig edition to contain the demo? This leads to a ton of duplicate code compared to starterkit-twig-demo. It's just a departure from the other editions, which have always been empty with the expectations that users install the starterkits separately.

Truer still, the CLI will now ask for the edition and then the starterkit, so baking them both into the edition reduces the value of the CLI process.

This comment has been minimized.

@bmuenzenmeyer

bmuenzenmeyer Jul 18, 2018

Member

This PR would be a ton smaller without all these files from the starterkit, too.

Show resolved Hide resolved packages/edition-twig/patternlab-config.json
Show resolved Hide resolved packages/engine-twig-php/lib/engine_twig_php.js
"version": "0.1.0",
"main": "lib/engine_twig_php.js",
"dependencies": {
"@basalt/twig-renderer": "^0.7.0",

This comment has been minimized.

@bmuenzenmeyer

bmuenzenmeyer Jul 18, 2018

Member

can we pin these dependencies?

This comment has been minimized.

@EvanLovely
Show resolved Hide resolved packages/edition-twig/patternlab-config.json
@@ -0,0 +1 @@
{"version":1,"timestamp":1531799832072,"graph":{"options":{"directed":true,"multigraph":false,"compound":false},"nodes":[{"v":"03-templates/01-homepage.twig","value":{"compileState":"clean"}},{"v":"03-templates/02-blog.twig","value":{"compileState":"clean"}},{"v":"03-templates/03-article-2col.twig","value":{"compileState":"clean"}},{"v":"03-templates/03-article.twig","value":{"compileState":"clean"}},{"v":"04-pages/00-homepage.twig","value":{"compileState":"clean"}},{"v":"04-pages/01-blog.twig","value":{"compileState":"clean"}},{"v":"04-pages/02-article.twig","value":{"compileState":"clean"}},{"v":"00-atoms/01-global/00-colors.twig","value":{"compileState":"clean"}},{"v":"00-atoms/01-global/01-fonts.twig","value":{"compileState":"clean"}},{"v":"00-atoms/01-global/02-animations.twig","value":{"compileState":"clean"}},{"v":"00-atoms/01-global/03-visibility.twig","value":{"compileState":"clean"}},{"v":"00-atoms/03-lists/00-unordered.twig","value":{"compileState":"clean"}},{"v":"00-atoms/03-lists/01-ordered.twig","value":{"compileState":"clean"}},{"v":"00-atoms/03-lists/02-definition.twig","value":{"compileState":"clean"}},{"v":"00-atoms/02-text/00-headings.twig","value":{"compileState":"clean"}},{"v":"00-atoms/02-text/02-blockquote.twig","value":{"compileState":"clean"}},{"v":"00-atoms/02-text/01-paragraph.twig","value":{"compileState":"clean"}},{"v":"00-atoms/02-text/03-inline-elements.twig","value":{"compileState":"clean"}},{"v":"00-atoms/02-text/04-time.twig","value":{"compileState":"clean"}},{"v":"00-atoms/02-text/05-preformatted-text.twig","value":{"compileState":"clean"}},{"v":"00-atoms/02-text/06-hr.twig","value":{"compileState":"clean"}},{"v":"00-atoms/04-images/00-logo.twig","value":{"compileState":"clean"}},{"v":"00-atoms/04-images/01-landscape-4x3.twig","value":{"compileState":"clean"}},{"v":"00-atoms/04-images/02-landscape-16x9.twig","value":{"compileState":"clean"}},{"v":"00-atoms/04-images/03-square.twig","value":{"compileState":"clean"}},{"v":"00-atoms/04-images/04-avatar.twig","value":{"compileState":"clean"}},{"v":"00-atoms/04-images/05-icons.twig","value":{"compileState":"clean"}},{"v":"00-atoms/04-images/06-loading-icon.twig","value":{"compileState":"clean"}},{"v":"00-atoms/04-images/07-favicon.twig","value":{"compileState":"clean"}},{"v":"00-atoms/05-forms/00-text-fields.twig","value":{"compileState":"clean"}},{"v":"00-atoms/05-forms/01-select-menu.twig","value":{"compileState":"clean"}},{"v":"00-atoms/05-forms/02-checkbox.twig","value":{"compileState":"clean"}},{"v":"00-atoms/05-forms/03-radio-buttons.twig","value":{"compileState":"clean"}},{"v":"00-atoms/05-forms/04-html5-inputs.twig","value":{"compileState":"clean"}},{"v":"00-atoms/06-buttons/00-buttons.twig","value":{"compileState":"clean"}},{"v":"00-atoms/07-tables/00-table.twig","value":{"compileState":"clean"}},{"v":"00-atoms/08-media/_00-video.twig","value":{"compileState":"clean"}},{"v":"00-atoms/08-media/_01-audio.twig","value":{"compileState":"clean"}},{"v":"01-molecules/00-text/00-byline.twig","value":{"compileState":"clean"}},{"v":"01-molecules/00-text/01-address.twig","value":{"compileState":"clean"}},{"v":"01-molecules/00-text/02-heading-group.twig","value":{"compileState":"clean"}},{"v":"01-molecules/00-text/03-blockquote-with-citation.twig","value":{"compileState":"clean"}},{"v":"01-molecules/00-text/04-intro-text.twig","value":{"compileState":"clean"}},{"v":"01-molecules/01-layout/01-two-up.twig","value":{"compileState":"clean"}},{"v":"01-molecules/01-layout/00-one-up.twig","value":{"compileState":"clean"}},{"v":"01-molecules/01-layout/02-three-up.twig","value":{"compileState":"clean"}},{"v":"01-molecules/01-layout/03-four-up.twig","value":{"compileState":"clean"}},{"v":"01-molecules/02-blocks/00-media-block.twig","value":{"compileState":"clean"}},{"v":"01-molecules/02-blocks/01-block-headline-byline.twig","value":{"compileState":"clean"}},{"v":"01-molecules/02-blocks/02-block-hero.twig","value":{"compileState":"clean"}},{"v":"01-molecules/02-blocks/03-block-thumb-headline.twig","value":{"compileState":"clean"}},{"v":"01-molecules/02-blocks/04-block-headline.twig","value":{"compileState":"clean"}},{"v":"01-molecules/02-blocks/05-block-inset.twig","value":{"compileState":"clean"}},{"v":"01-molecules/03-media/00-figure-with-caption.twig","value":{"compileState":"clean"}},{"v":"01-molecules/04-forms/01-comment-form.twig","value":{"compileState":"clean"}},{"v":"01-molecules/04-forms/00-search.twig","value":{"compileState":"clean"}},{"v":"01-molecules/04-forms/02-newsletter.twig","value":{"compileState":"clean"}},{"v":"01-molecules/05-navigation/01-footer-nav.twig","value":{"compileState":"clean"}},{"v":"01-molecules/05-navigation/00-primary-nav.twig","value":{"compileState":"clean"}},{"v":"01-molecules/05-navigation/02-breadcrumbs.twig","value":{"compileState":"clean"}},{"v":"01-molecules/05-navigation/03-pagination.twig","value":{"compileState":"clean"}},{"v":"01-molecules/05-navigation/04-tabs.twig","value":{"compileState":"clean"}},{"v":"01-molecules/06-components/00-social-share.twig","value":{"compileState":"clean"}},{"v":"01-molecules/06-components/01-accordion.twig","value":{"compileState":"clean"}},{"v":"01-molecules/06-components/02-single-comment.twig","value":{"compileState":"clean"}},{"v":"01-molecules/07-messaging/00-alert.twig","value":{"compileState":"clean"}},{"v":"02-organisms/00-global/00-header.twig","value":{"compileState":"clean"}},{"v":"02-organisms/00-global/01-footer.twig","value":{"compileState":"clean"}},{"v":"02-organisms/01-article/00-article-body.twig","value":{"compileState":"clean"}},{"v":"02-organisms/02-comments/00-comment-thread.twig","value":{"compileState":"clean"}},{"v":"02-organisms/03-sections/00-latest-posts.twig","value":{"compileState":"clean"}},{"v":"02-organisms/03-sections/01-recent-tweets.twig","value":{"compileState":"clean"}},{"v":"02-organisms/03-sections/02-related-posts.twig","value":{"compileState":"clean"}},{"v":"03-templates/00-layouts/00-site.twig","value":{"compileState":"clean"}},{"v":"03-templates/00-layouts/01-page-1col.twig","value":{"compileState":"clean"}},{"v":"03-templates/00-layouts/01-page-2col.twig","value":{"compileState":"clean"}},{"v":"04-pages/00-homepage~emergency.json","value":{"compileState":"clean"}}],"edges":[{"v":"04-pages/00-homepage~emergency.json","w":"04-pages/00-homepage.twig","value":{}}]}}

This comment has been minimized.

@sghoweri

sghoweri Jul 18, 2018

Contributor

Why can鈥檛 this be gitgnored?... Directly committing auto generated assets = 馃槹

.travis.yml Outdated
@@ -1,6 +1,7 @@
language: node_js
before_install:
- npm i -g npm

This comment has been minimized.

@sghoweri

sghoweri Jul 18, 2018

Contributor

Sorry, another PL Node newb question here: any particular reason why aren鈥檛 we using Yarn + Lerna Workspaces here?

@@ -3,7 +3,7 @@
"lerna": "3.0.0-beta.21"
},
"scripts": {
"bootstrap": "lerna bootstrap --hoist tap --hoist eslin* --hoist husky --hoist prettier --hoist pretty-quick",
"bootstrap": "lerna bootstrap --no-ci --hoist tap --hoist eslin* --hoist husky --hoist prettier --hoist pretty-quick",

This comment has been minimized.

@sghoweri

sghoweri Jul 18, 2018

Contributor

Shouldn鈥檛 we move these to the lerna.json config in the root so these flags always get added?

{
"engines": {
"twig": {
"namespaces": [

This comment has been minimized.

@sghoweri

sghoweri Jul 18, 2018

Contributor

@EvanLovely three big questions here:

  1. Are these recursive by default?
  2. How do we configure PHP Twig extensions?
  3. For that matter, how would we configure any of the other Twig Renderer config options:
    https://github.com/basaltinc/twig-renderer/blob/master/config.schema.json

This comment has been minimized.

@EvanLovely

EvanLovely Jul 19, 2018

Member
  1. Not by default, but that's just a recursive: true away. I want to update the Twig StarterKit demo to change things like @atoms/04-folder/01-item.twig to @atoms/01-item.twig as well.
  2. Just set engines.twig.alterTwigEnv - you can see where the config is passed in here
  3. I want to review the config a bit more on TwigRenderer before I lock it down (and release 1.0.0). Then we just pass more config in like the above link.
"meta": "./source/_meta/",
"annotations": "./source/_annotations/",
"styleguide": "dist/",
"patternlabFiles": {

This comment has been minimized.

@sghoweri

sghoweri Jul 18, 2018

Contributor

@EvanLovely woah woah woah - hold up! I鈥檓 like 99% sure we need to swap these out with out verbatim with the Twig versions of these (and add this to the monorepo here):

I already went ahead and ported these original Twig Templates over/ updated to work with the latest CSS / JS changes in the UIKit Workshop if this helps:
https://github.com/bolt-design-system/bolt/tree/master/packages/styleguidekit-twig-default

This comment has been minimized.

@EvanLovely

EvanLovely Jul 19, 2018

Member

I think it's working right now as this supports rendering Twig and Mustache files. I'm fine with getting Twig ones in there as well.

@hdwebpros

This comment has been minimized.

hdwebpros commented Jul 18, 2018

I did a light test of this. It seems initially to work great.

@EvanLovely

This comment has been minimized.

Member

EvanLovely commented Jul 20, 2018

Hey @sghoweri - take a peak at 7c2e376 for how one would add custom Twig Extensions, functions, globals, and more.

@EvanLovely

This comment has been minimized.

Member

EvanLovely commented Jul 20, 2018

@bmuenzenmeyer I've removed the starterkit from the twig edition in 09d135a

@EvanLovely

This comment has been minimized.

Member

EvanLovely commented Jul 20, 2018

So @bmuenzenmeyer & I discussed where the config should live and think that it be where it currently is. If someone were to pull in a starterkit that used @components and @layouts and the config at the root had @atoms etc, then it'd be relatively straightforward to update the config. I could automatically register every folder under source/_patterns as a namespace, but I think it's better to have it be less of a black box and more clear where to change it. Also, this opens up a lot of opportunity for using templates in a different folders as each namespace is an array of folders, enabling config such as:

{
  "id": "components",
  "paths": [
      "source/_patterns/03-components",
      "node_modules/@basalt/awesome-component-collection/src"
   ]
}

We also discussed that allowing the config to be formats other than json, ie (patternlab-config.js) lets individuals creatively import and create config as it fits them best. See #900

@EvanLovely

This comment has been minimized.

Member

EvanLovely commented Jul 20, 2018

Still need to fix the header & footer showing undefined on individual pattern pages. To see, spin it up and click on a single pattern.

@sghoweri

This comment has been minimized.

Contributor

sghoweri commented Jul 21, 2018

Still need to fix the header & footer showing undefined on individual pattern pages. To see, spin it up and click on a single pattern.

@EvanLovely I think my #902 PR addresses this (or at least I couldn鈥檛 reproduce this issue on my PR branch) 馃

@aleksip

This comment has been minimized.

Member

aleksip commented Jul 21, 2018

Tried again, following the instructions after cloning the feature/engine-twig-php branch from the repo.

This time Pattern Lab was fired up in the browser! There were several warnings/exceptions about missing files/directories. packages/edition-twig/source contains only a _meta dir with Mustache header and footer files. I guess I should try installing a StarterKit? How do I do this?

The 'All' page shows undefined for the header and footer. Replaced Mustache header and footer files with Twig ones. No visible effect. Starting Pattern Lab again results in the Mustache files being copied under _meta again.

Added a source/_patterns/00-atoms/headings.twig file, this shows up in the menu, but can't see the contents.

@aleksip

This comment has been minimized.

Member

aleksip commented Jul 21, 2018

Merged #902 and now I get a nice Pattern Lab with the Twig Starterkit Demo files! 馃帀

Only with Chrome though. Blank pages with Firefox and Safari. But I guess this is a different issue?

@sghoweri

This comment has been minimized.

Contributor

sghoweri commented Jul 21, 2018

@aleksip nice! That's the whole point of that supplemental PR :-)

Regarding that blank screen issue you mentioned, while I have seen a few misc JS / CSS bugs in general on the new version of PL Node's UIKit over the past couple weeks (been using it in Bolt after fixing a bunch -- follow-up PR pending), I haven't seen that specific blank page issue on my end.

Here's a screenshot of Chrome, Safari, IE 11, and Firefox -- are you seeing any console errors on your end? ...maybe a different issue?

image

@aleksip

This comment has been minimized.

Member

aleksip commented Jul 21, 2018

Followed the instructions in the repo README and installed edition-node and starterkit-mustache-demo with the same results: works in Chrome but blank pages (apart from the Pattern Lab navbar) in Firefox and Safari. So this is not related to the Twig PHP PRs. Maybe it is something to do with my setup, don't know. No errors in Safari console, Firefox console has this warning:

An iframe which has both allow-scripts and allow-same-origin for its sandbox attribute can remove its sandboxing. 127.0.0.1:3000

@EvanLovely

This comment has been minimized.

Member

EvanLovely commented Jul 21, 2018

I鈥檝e also had issues with Firefox and believe it to not be from this PR. Strangely enough, Inspeciting Element in it changes it from all white to rendering properly.

@aleksip @sghoweri - I really appreciate you checking this and helping. Let鈥檚 make this happen!

@sghoweri

This comment has been minimized.

Contributor

sghoweri commented Jul 22, 2018

@EvanLovely ohhhhhhhhh I know that PL UIkit bug. Heck, I think I already fixed that in Bolt... lemme confirm and if that's the case I'll get a separate PR up!

BTW I just pushed up a couple small things in my #902 PR after doing some further testing using the mustache engine -- should be in better shape now!

@sghoweri

This comment has been minimized.

Contributor

sghoweri commented Jul 22, 2018

@aleksip PR up with the Safari / Firefox fix! #903

chore: add example of the Twig debug extension to alter-twig.php to s鈥
鈥ow how to toggle on/off Twig debuggin
@hdwebpros

This comment has been minimized.

hdwebpros commented Jul 31, 2018

Please let me know if I did something out of order. I did
git clone https://github.com/pattern-lab/patternlab-node.git
cd patternlab-node
git checkout feature/engine-twig-php
npm install
npm run bootstrap
cd packages/edition-twig
npm install
*create test twig pattern and folder in source/_patterns folder
*Rename meta head and foot to .twig
*Create remaining missing assets / folders to avoid nag errors

I then notice some things with the config file:

  • I noticed the config compiled as a single line. Maybe there is a PR already that tried to fix that?
  • Even though there were no patterns (besides what I just made) the config has atoms, etc.

Then, I format the json file and manually name my namespaces.
Then I run npm start

Those should be adequate steps, right?

If so, I get

TypeError: Cannot read property 'render' of undefined
    at Promise (C:\websites\fail\patternlab-node\packages\engine-twig-php\lib\engine_twig_php.js:79:10

and that line of code is
.render(patternPath, data)

More similar errors that show are

error during header render()
error during header render()
error during footer render()
error during footer render()
Error building buildFooterHTML
Error building buildFooterHTML

Those errors are surrounded by the rendor error I mentioned above. And then it ends with

Built Pattern Lab front end
Built Pattern Lab front end
Pattern Lab is watching for changes to files under ./source/
Pattern Lab is watching for changes to files under ./source/
Pattern Lab is being served from http://127.0.0.1:3000
Pattern Lab is being served from http://127.0.0.1:3000

Weird that everything is duplicated.

I swear I tested this earlier and it worked.

Did I do something wrong?

@bmuenzenmeyer

This comment has been minimized.

Member

bmuenzenmeyer commented Jul 31, 2018

@hdwebpros thanks for the thorough testing and report. I can speak to the below right now, haven't vetted much of the actual rendering yet - but it is a promising WIP

I noticed the config compiled as a single line. Maybe there is a PR already that tried to fix that?

#894 is the issue tracking that. no one has taken it on yet.

EvanLovely added some commits Aug 8, 2018

Merge pull request #907 from sghoweri/feature/add-twig-debug-ext-example
Add Example of Toggling Twig Debug Mode in New Twig PHP Engine
@stale

This comment has been minimized.

stale bot commented Oct 7, 2018

It's hard to keep track of everything. This issue has been automatically marked as stale because it has not had recent activity, neither from the team nor the community. It will be closed if no further activity occurs. Please consider adding additional info, volunteering to contribute a fix for this issue, or making a further case that this is important to you, the team, and the project as a whole. Thanks!

@stale

This comment has been minimized.

stale bot commented Oct 21, 2018

Issue closed after going stale. It can be re-opened if still relevant.

@stale stale bot closed this Oct 21, 2018

@sghoweri sghoweri reopened this Oct 21, 2018

@stale stale bot removed the needs response 馃 label Oct 21, 2018

@bmuenzenmeyer

This comment has been minimized.

Member

bmuenzenmeyer commented Oct 26, 2018

Just chiming in here to gauge interest in landing it. I need help from folks better versed in PHP. @sghoweri @EvanLovely

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment