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

Support slim require #26

Merged
merged 9 commits into from
Aug 15, 2015
Merged

Conversation

bitencode
Copy link
Contributor

HI @cognitom,
Here are a set of changes and a test that support using the optional plug-ins the come with slim. Specifically, I added and test the 'slim/include' plug-in that allows including common markup from multiple slim files.

@bitencode
Copy link
Contributor Author

I see that today while I was working on this, you merge a pull request from @dmke that adds -l, and -t options. I wrote this as a generic require so that users of gulp-slim could require any ryby plug-in/library they needed to. If you would prefer to go with a more simple interface I can change this to just be a -i option instead.

… file currently being processed (for including files in the same directory).
@jvmccarthy
Copy link

Hi @cognitom, to support including slim files in the same directory, I also added a chdir option. This option allows for changing the working directory of slim to the file that is currently being processed. That way, if the file includes other files in the same directly, they are correctly picked up. (This appears to work automatically when passing a file to the slim engine instead of just the file contents.)

@Nu-hin
Copy link

Nu-hin commented Apr 19, 2015

I agree, this is a very welcome addition. Please consider merging.

@phyllisstein
Copy link

👍 Would be great to see this merged in!

@cognitom
Copy link
Owner

@bitencode @jvmccarthy @Nu-hin @phyllisstein sorry for my long absence on this repo 🙀
If one of you still have an interest on this PR, I'd like to invite you to the collaborator. How about?
(before merge this, it needs to resolve the conflict, I think)

@jvmccarthy
Copy link

Thanks @cognitom. The merge didn't seem that bad, but now I'm running into a failing test (should include additional file with include plugin). I'll see if I can get this test fixed but have pushed it a new branch, support_slim_require_failing, in case someone else wants to look at it.

Thanks for offering to make me a collaborator. Actually, we've moved on from slim to jade. In my opinion, jade is simply the templating engine to use with node.js tooling. I'll try to get this PR in but no longer use gulp-slim.

@cognitom
Copy link
Owner

@jvmccarthy thanks!
Yeah, agreed. In the JavaScript ecosystem, jade is better option. gulp-slim is needed only when we interact with Rails. BTW, recently I prefer the approach like react.js instead of the template engines.

John V McCarthy added 2 commits August 14, 2015 23:36
…upport_slim_require

Conflicts:
	coffee/index.coffee
	test/fixtures/include.slim
	test/main.coffee
@jvmccarthy
Copy link

@cognitom Ok, I've got the merge resolved. Thanks for the pointer to react.js. I've read up a bit on it and should give it a try. (Currently using angular and jade templates with webpack. Enjoying webpack so far.)

@cognitom
Copy link
Owner

@jvmccarthy Looks nice! Thank you. I'll merge and publish it as v0.3.0 soon.

BTW, don't miss it, too. Riot.js is my favourite ;)
https://muut.com/riotjs/

cognitom added a commit that referenced this pull request Aug 15, 2015
@cognitom cognitom merged commit dd31be8 into cognitom:master Aug 15, 2015
@jvmccarthy jvmccarthy deleted the support_slim_require branch August 15, 2015 05:57
@cognitom
Copy link
Owner

Done.
https://www.npmjs.com/package/gulp-slim

Thanks again @bitencode and @jvmccarthy !

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