Skip to content
This repository was archived by the owner on Feb 7, 2023. It is now read-only.

Allowed for use of Param with Pattern Includes - #65

Closed
coding-stuff wants to merge 1 commit into
pattern-lab:masterfrom
coding-stuff:master
Closed

Allowed for use of Param with Pattern Includes#65
coding-stuff wants to merge 1 commit into
pattern-lab:masterfrom
coding-stuff:master

Conversation

@coding-stuff

Copy link
Copy Markdown
Contributor

Created two new methods:

::getPatternIncludeData line 185

Which extracts param from pattern include call

and

::renderPattern line 278

which replaces param name with actual param value.

I also modified the ::loadFile method so that it makes use of both functions before returning patterns.

Now you can pass variables to patterns when they are included like this:

{{> patternType-patternName(param: param_value, param2: value 2 ...) }}

For parse to work properly, however, commas and parentheses must be escaped with a back slash (example: This is a (string) , separated with a comma )

I've tested/debugged this multiple times, and it works, check it out

Created two new methods:

::getPatternIncludeData line 185

Which extracts param from pattern include call

and

::renderPattern line 278

which replaces param name with actual param value.

I also modified the ::loadFile method so that it makes use of both functions before returning patterns.

Now you can pass variables to patterns when they are included like this:

{{> patternType-patternName(param: param_value, param2 ...) }}

For parse to work properly, however, commas and parentheses must be escaped with a back slash (example: This is a \(string\) \, separated with a comma )
@dmolsen

dmolsen commented Nov 28, 2013

Copy link
Copy Markdown
Member

@coding-stuff -

Thanks much for the PR :) I've merged this locally and taken a look. My two quibbles:

  1. I think we can take an opinion and say parameters are separated from a pattern name by a space and just use explode() to get the pattern name and parameters. Then we could just trim off the first and last character of the parameters string to drop the parentheses. So no escaping those.
  2. I also think strings with spaces should be delimited by quotes (single or double). This would make it easier to build off of the JSON that might already exist to describe the values for a pattern. Users also wouldn't have to worry about escaping commas. This might make for a more difficult regex but I think it'd be worth it.

I'm going to try to add that stuff into your work tomorrow. I think it's also a good time to add in the style modifier. Not full KSS support but since we're hacking this file already...

I really appreciate you suggesting the idea but, more importantly, doing the legwork to implement it. It's going to be a really popular feature and I'm sure you've scratched a lot of itches with this PR. This PR will stay open until I merge my feature branch with dev and push to GitHub. Please don't take that as us ignoring it. Hopefully it won't be open long.

@coding-stuff

Copy link
Copy Markdown
Contributor Author

@dmolsen

This program is really helpful for my projects, and is just valuable in general because it makes the concept of atomic design tangible, which promotes standardization (which IMO front end dev. needs A LOT more of), so I'm glad to contribute in any way possible. That's the long way of saying "my pleasure". Anyway just a few things regarding your feedback:

I think we can take an opinion and say parameters are separated from a pattern name by a space and just use explode() to get the pattern name and parameters. Then we could just trim off the first and last character of the parameters string to drop the parentheses. So no escaping those.

Right, I know escaping parentheses seems unnatural as escaping commas and I actually think that, the second solution can solve the escape problem for both parentheses and commas. However, I think the above solution (exploding by spaces), which was my first inclination, because it seemed the easiest, could backfire. This is because IMO many developers are used to the:

functionName(param, param, param...)

syntax. Even though

{{ > patternType-patternName (param1: a, param2: b) }}

isn't exactly a function, it kind of feels like it, and i thought it might be easy to forget to add the space,which could possibly inconvenience someone. (Not that escaped commas and parentheses wouldn't do the same thing, but this was my thought process at the time). And since generating the site over and over to find errors in param syntax can become tedious. This is why i just regexed everything between parentheses because it avoids the potential problem I just described by allowing no spaces or one thousand (unfortunately it creates another problem which is the need to escape parentheses) but i think your second answer solves that:

I also think strings with spaces should be delimited by quotes (single or double). This would make it easier to build off of the JSON that might already exist to describe the values for a pattern. Users also wouldn't have to worry about escaping commas. This might make for a more difficult regex but I think it'd be worth it.

In general, there is no reason to not delimit strings with quotes (whether they have spaces or not). First because it seems natural and second it would solve the escape problem as a whole, because, if we force param name to follow variable naming conventions (no special chars, including commas and parentheses ), then the param value would be the only place we could expect to see a comma or parentheses (as a string), and our regex would ignore anything in quotes. example:

(param: " This (parentheses) and (,) shall be ignored ", param2: "value 2")

of course this means that we have to escape quotes, or just use single quotes with double quote delimiters (or vice versa) but it's only one escape vs two. And it is more natural to programming language, unlike escaped commas and parentheses.

So let me know your thoughts, the sooner it can be implemented the better.

@dmolsen

dmolsen commented Nov 28, 2013

Copy link
Copy Markdown
Member

@coding-stuff -

While I don't think there will be much confusion it's always good to handle an easy-to-make mistake. I think exploding on ( will work too.

@dmolsen

dmolsen commented Nov 29, 2013

Copy link
Copy Markdown
Member

@coding-stuff -

Looking at the Mustache spec I realized spaces were allowed in param names. I took a second crack at the problem and you can find it in the feature-patternparams-take2 branch. Unfortunately, this meant nuking most of the work you had done. Rather than regular expressions I built a parser for the parameters. Hopefully this offers flexibility for folks who use the feature. I almost went with JSON but PHP's JSON parser is really strict and I know I'd get annoyed having to add double quotes everywhere. Feel free to play with it and let me know if you can break it.

The following should all be supported:

{{> atoms-color:foo }} to replace {{ styleModifer }} in the called pattern
{{> atoms-color(foo: bar) }}
{{> atoms-color (foo: bar) }}
{{> atoms-color ("foo baz": "bar") }} to replace {{ foo baz }} in the called pattern
{{> atoms-color (foo: "baz has a \" quote") }}
{{> atoms-color (foo: "baz has a ' quote") }}
{{> atoms-color (foo: 'baz has a \' quote') }}
{{> atoms-color (foo: 'baz has a " quote') }}
{{> atoms-color (foo: "bas has a ) parens") }}
{{> atoms-color (foo: bar, baz: fuz) }}

Again, thanks for the original work. We'll make sure to give you credit.

@coding-stuff

Copy link
Copy Markdown
Contributor Author

@dmolsen

excellent! Will give it a shot for sure. I read through the source and it looks pretty solid, especially that param parser (that thing seems strong enough to make into a stand alone function/class sometime down the line). Anyway, good stuff! Appreciate the cred. also - will try this out the first chance I get.

one question though, what inspired the syntax

atoms-color:foo

for the {{ styleModifer }}, as opposed to

atoms-color (styleModifier: "my-style") ?

@dmolsen

dmolsen commented Nov 29, 2013

Copy link
Copy Markdown
Member

@coding-stuff -

That's related to issue #34. I just wanted to build in a shortcut in case we really implemented that. Your second example would also work.

@dmolsen

dmolsen commented Nov 30, 2013

Copy link
Copy Markdown
Member

The rewritten version of this feature has been merged into dev. I'm closing just to get the overall issues list under control.

@dmolsen dmolsen closed this Nov 30, 2013
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants