Skip to content

Conversation

@sghoweri
Copy link
Contributor

@sghoweri sghoweri commented Jul 21, 2018

Hey @EvanLovely and @bmuenzenmeyer -- I took a crack at trying to address a bunch of the remaining todos from #897 and wound up with a couple small things to tweak + one big update I wanted to run by you guys 😄


1. Fixed Broken Demos:

Given this particular commit where we're removing the original set of Twig Starterkit Demo files, I realized locally that I had no way of easily testing out and validating everything else was working SO, I went back in and added the @pattern-lab/starterkit-twig-demo dependency + updated the Twig-specific patternlab-config.json in to point at the NPM installed assets (at least for the time being).

2. Ported Over, Updated Styleguidekit Twig Templates

Having the ability to customize the templates that Pattern Lab wraps individual components / sections / View All views in (and do so in your project's native tech stack) was one of the big things nagging me with the initial Twig PHP PR so I've gone ahead and ported the whole lot over from https://github.com/pattern-lab/styleguidekit-twig-default (after updating to match a ton of classname changes)

3. Updated PL Node to Support Non-Inlined UI Templates

In a nutshell, after porting over the Styleguidekit Twig templates, I realized that the way PL Node's internals are currently setup require the templates used to general Pattern Lab's core UI to be inlined; template paths just don’t work!

Unfortunately, that could mean having to add massive hacks and workarounds to the new Twig PHP rendering engine so... I also took the liberty at taking a crack at getting PL Node to be a little bit more flexible when it comes to handling those internal UI templates without inadvertently causing a major breaking API change etc.

Basically, the way I have this change set up, if the UIkit templates specified use a longhand object config (to explicitly specify a path), the internal logic passes that config path along to all the internal Pattern.createEmpty calls, otherwise continue to handle things way they had been (inline the template specified for mustache).

Thoughts? Ideas? Questions?

engelfrost and others added 8 commits July 10, 2018 21:26
fix(package): Allow .json extension on annotations file (issue pattern-lab#836)
…epo, updating to match more recent UIkit JS and CSS updates
…st Twig PHP compiles properly; update demo config to point at assets for the time being
…templates not be inlined (aka allow Twig namespaced paths to point at these templates vs inlining the contents)
bolt-bot added 3 commits July 21, 2018 20:18
…; fix typo / formatting bug causing mustache loader to not compile base UI templates as expected
Copy link
Member

@bmuenzenmeyer bmuenzenmeyer left a comment

Choose a reason for hiding this comment

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

I had no way of easily testing out and validating everything else was working SO

This is the exact reason @geoffp and I ported the concept of development-editions into the monorepo - editions that do not get published but can be used locally to test more end to end functionality and engine nuances. I also plan to run test suites off of these.

I disagree with including a starterkit within the edition.

The editions that get bundled edition-node edition-node-gulp and as part of #897 edition-node-twig should be clean (devoid of patterns). One of the main reasons for this is that the CLI will ask users 1. what edition they want, and 2. what starterkit they want. Baking in a starterkit into the edition blunts the effectiveness of this feature.

I like what you are doing with the uikit template resolution - very smart and a nice enhancement.

@sghoweri
Copy link
Contributor Author

sghoweri commented Jul 24, 2018

@bmuenzenmeyer are those general testing steps (going through the CLI to locally install what’s needed) documented somewhere?

Didn’t even occur to me to go through the CLI 🤦‍♂️ - I’ll make sure to remove the Starterkit dependency!

@sghoweri
Copy link
Contributor Author

@bmuenzenmeyer Updated to remove the Twig starterkit-related changes from this PR!

@EvanLovely
Copy link
Member

Thanks you two! I’ll look into all this more before the end of the week. Really excited to get this all put together!

@bmuenzenmeyer
Copy link
Member

Thanks for reverting those changes @sghoweri - appreciate your flexibility! I'd like Evan to bless this PR before it goes in. Sounds like he's committed to do so, I am assigning him.

@sghoweri
Copy link
Contributor Author

👍🏼

@EvanLovely
Copy link
Member

@sghoweri This all looks good! Now that @basalt/twig-renderer has a .renderString() method, would that help? Or should we just merge this as-is?

@sghoweri
Copy link
Contributor Author

sghoweri commented Aug 8, 2018

@EvanLovely Thanks!

I'd still recommend merging as-is. The renderString() method might help us here, however I'd still very much prefer to see us de-tangling some of Pattern Lab's quirks so we can keep moving things forward.

In this particular instance, this allows any internal or external template getting rendered by Pattern Lab to be resolvable via a path vs previously assuming the template rendering engine (or source of the template itself) renders like the original mustache templates Pattern Lab started off with.

@stale
Copy link

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!

@bmuenzenmeyer
Copy link
Member

@EvanLovely bump on this. I've lost track of where we are at withthis.

Cc @sghoweri

@EvanLovely
Copy link
Member

I’ll admit I’ve lost track about too. Need to clear out a few hours and look it all over.

@sghoweri
Copy link
Contributor Author

@EvanLovely lemme know if there's anything you need help with on this!

@stale
Copy link

stale bot commented Dec 14, 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
Copy link

stale bot commented Dec 29, 2018

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

@stale stale bot closed this Dec 29, 2018
@sghoweri sghoweri reopened this Dec 29, 2018
@sghoweri
Copy link
Contributor Author

sghoweri commented Feb 2, 2019

@EvanLovely @bmuenzenmeyer I’m going to move forward on at least merging these updates into Evan’s branch — then we just have one Twig Renderer-related PR we need to juggle 🙂

@sghoweri sghoweri merged commit 36a63f7 into pattern-lab:feature/engine-twig-php Feb 2, 2019
antonia-rose pushed a commit to quelltexterin/nemo-uikit-workshop that referenced this pull request Apr 12, 2023
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.

5 participants