Passing multi-line mixin arguments throws parse errors #700

Closed
csuwildcat opened this Issue Jul 6, 2012 · 15 comments

Comments

Projects
None yet
7 participants

This is a current mixin I use that is becoming unwieldy because of the inability to pass multi-line arguments. The mixin creates a ribbon and the options object lets you pass in an array of button objects that are added as button elements inside the ribbon. With as little as three buttons it begins to become a hassle to manage on a single line :(

mixin ribbon('Question', {id: 'question_ribbon', buttons: [{ icon: 'twitter-bird-16', modal: 'share_twitter', title: 'Share on Twitter' }, { icon: 'google-plus-16', modal: 'share_google_plus', title: 'Share on Google Plus' }, { icon: 'facebook-16', modal: 'share_facebook', title: 'Share on Facebook' }]})
Contributor

tj commented Jul 6, 2012

why not :

twitter = { icon: 'twitter-bird-16', modal: 'share_twitter', title: 'Share on Twitter' }
google = { icon: 'google-plus-16', modal: 'share_google_plus', title: 'Share on Google Plus' }
...
+ribbon('Question', { id: 'stuff', buttons: [twitter, google] })

etc if you're going that route

Hard to call that a solution, the buttons can take up to 5 options, which circles back to the issue that multi-line variable declarations also throw errors. Do you have a solution for when a mixin needs objects and putting each object on one line becomes an issue?

I'm reporting these issues because I care, and very much enjoy your Jade for Node lib, just trying to point out where it can be even better.

Contributor

tj commented Jul 6, 2012

totally, im not trying to be rude or anything I just think that if there's that much js in a template that i's "probably" the wrong route to go

What route would you advise for such things? I certainly don't want to pollute my data layer with mark-up related code, so I thought this was the best choice, are there other mechanisms in Jade that offer similar markup componetization/re-use?

Contributor

tj commented Jul 6, 2012

it really depends, there's no correct answer or anything, personally we build little declarative widgets on the client, so we dont really mix and match them in jade on the server

Contributor

chowey commented Jul 6, 2012

As a note, my pull request at #629 would fix this.

I don't see that there is a reason NOT to implement it if it is already coded. Maybe the code just needs review?

Contributor

tj commented Jul 6, 2012

ah yeah I didn't see that, it's always more code to break/maintain though as well

I'd really think if chowey's code is good that this would be an excellent addition to the library. Also, I don't feel that fear of adding code because it has to be maintained is a valid reason. Under that logic all new activity on this project should cease, right?

Contributor

tj commented Jul 6, 2012

yeah but you need to pick and choose features of course, not just blindly merge things that people want without fully understanding the use-cases etc, that's why I nitpick so much and ask people about uses and maybe how those uses might not be appropriate etc

True, that does make sense. I would highlight that there are many
outstanding tickets asking for this feature, perhaps that indicates it
warrants inclusion?
On Jul 6, 2012 12:36 PM, "TJ Holowaychuk" <
reply@reply.github.com>
wrote:

yeah but you need to pick and choose features of course, not just blindly
merge things that people want without fully understanding the use-cases
etc, that's why I nitpick so much and ask people about uses and maybe how
those uses might not be appropriate etc


Reply to this email directly or view it on GitHub:
visionmedia#700 (comment)

+1 on this. mostly so that i can make my mixins easier to maintain by allowing an options argument instead of having to maintain a specific order for args. ie:

mixin textfield(type, label, placeholder)

would be much nicer as:

mixin textfield(options)

so that things are much clearer when i use it later:

+textfield({
  type        : 'email',
  label       : 'Email',
  placeholder : 'aristotle@segment.io'
})

as opposed to the current:

+textfield('email', 'Email', 'aristotle@segment.io')

three arguments alone is already impossible to remember ordering on, and that's not even the maximum i'm using for mixins.

duro commented Sep 26, 2012

+1 for this. I'm using it the same way @ianstormtaylor is

Owner

ForbesLindesay commented Apr 14, 2013

Will be fixed by #962

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