Skip to content
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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Styleguidekit Assets Default Dependency #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sghoweri
Copy link

@sghoweri sghoweri commented May 2, 2017

Corresponds with drupal-pattern-lab/edition-php-twig-standard#1 so dependencies are centralized in one place (particularly code that's are required by the project itself vs a specific sub-dependency).

Adding this in to the main PL Twig Edition fork separately

@EvanLovely
Copy link

I think we might want to leave that in. Also, most of the work we want in has to do with the twig engine and pl-php-core. Let's focus on that first. I could see us coming back around to this after we get some other work shipped.

@EvanLovely
Copy link

OK, we're doing this 👍

@EvanLovely
Copy link

Wait: is removing the dependency necessary? What about adding a repositories object similar to other forks?

@sghoweri
Copy link
Author

sghoweri commented May 2, 2017

@EvanLovely I had the exact same thought originally but here's the thing:

A. we've already added that dependency in the main PL Twig Edition fork with the repositories object pointing to our fork which just got merged in

B. In my opinion, I don't think this should have ever been added as a dependency to this repo from the getgo (hence the PR). The styleguidekit-assets-default code isn't Twig specific from what I understand so I feel like that should go in either the primary composer.json file of your project (hence the PL Twig Edition fork) or maybe as a dependency to patternlab-php-core...? I'm also humoring the idea of breaking apart the styleguidekit-assets-default repo into smaller parts that we can rewrite one part at a time so the easier it is to swap out the version, the better.

I'm super flexible if you feel strongly one way or the other but for what it's worth, everything appeared to work just fine locally when I switched my PR branch on the main PL Twig Edition to use this PR feature branch (ie. only reference the styleguidekit-assets-default dependency in the PL Twig Edition composer.json file)

Thoughts?

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.

3 participants