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

ListItems are not compiled when included using a pattern parameter #457

Closed
gfnool opened this issue Aug 31, 2016 · 24 comments
Closed

ListItems are not compiled when included using a pattern parameter #457

gfnool opened this issue Aug 31, 2016 · 24 comments
Assignees
Labels

Comments

@gfnool
Copy link

gfnool commented Aug 31, 2016

I am using Pattern Lab Node v2.4.0 on Mac, with Node v6.2.2, using the Gulp Edition.

Expected Behavior

When a pattern containing a listItems loop is included in another pattern, the list loop should be printed out, even if some data are overwrite from the parent pattern

Actual Behavior

The included pattern do not show the list

Steps to Reproduce

create a child pattern like this:

{{ title }}
<ul>
{{# listItems.four }}
   <li>{{ headline.short }}</li>
{{/ listItems.four }}
</ul>

import the child pattern in a parent like this:

{{> atoms-list(title: "title") }}

the parent pattern it will not contain the list

Here's a screenshot
schermata 2016-08-31 alle 14 22 10

@dennisfrank
Copy link

Yep. Having the same issue issue (Pattern Lab Node v2.5.1 on Mac, with Node v5.3.0, using a custom Grunt Edition). 😕

@gael-boyenval
Copy link
Contributor

+1 same here.

@st3phan
Copy link

st3phan commented May 18, 2017

Too bad this is still an issue. Any chance you can fix this?

Patternlab version: "patternlab-node": "2.9.1"

atoms/icon.mustache

<div class="icon">Icon {{#test}}test{{/test}}</div>

molecules/button.mustache

<button>{{> atoms-icon(test: true) }}</button>

organisms/component.mustache

<div>
    The icon in the button does not render when we set a data parameters:
    {{> molecules-button(modifier: true) }}
</div>
<div>
    The icon in the button does render without the data parameter:
    {{> molecules-button }}
</div>

@raphaelokon
Copy link
Contributor

raphaelokon commented May 18, 2017

@bmuenzenmeyer Is this something I can help with? Had some people asking me about this specific bug …

@bmuenzenmeyer
Copy link
Member

bmuenzenmeyer commented May 18, 2017

hey @raphaelokon

I think these are two separate use cases here - so it's a bit much to unpack.

Either way, more careful processing of these nuances is needed...

What I've tried to do is isolate test cases into their own files in a starterkit when PL PHP parity is a concern, and also of course have some unit test coverage.

Which use case would you like to help with, or what are you most comfortable with between the two? Both entail isolating the bug in a failing unit test and then rolling up your sleeves into decomposePattern

@raphaelokon
Copy link
Contributor

@bmuenzenmeyer I'd pickup the list item parsing one
/cc @dennisfrank

@bmuenzenmeyer
Copy link
Member

👍 feel free to ask any questions along with way here or on gitter

@bmuenzenmeyer
Copy link
Member

@st3phan can you upgrade and retest? I think fixing #539 fixed your issue - though likely the OP use case is still a problem.

@st3phan
Copy link

st3phan commented May 19, 2017

Upgraded from 2.9.2 to 2.9.3 and it's now working. #539 looks identical indeed. Thanks!

@bmuenzenmeyer
Copy link
Member

@st3phan
Copy link

st3phan commented May 19, 2017

ROFL 💯

@raphaelokon
Copy link
Contributor

raphaelokon commented May 22, 2017

@bmuenzenmeyer Can we have a proper test case for this? And I guess we want to land this in v2.9.x, correct?

@bmuenzenmeyer
Copy link
Member

Hey @raphaelokon

Not sure if you are asking me for steps to reproduce, the process I would undergo, or for a rundown of what I think the problem is from the OP, so I will give you my braindump approach.

My first steps would be to attempt to recreate the OP's issue using the supplied patterns and circumstances, asking him directly for more information as needed. (It happened so long ago I cannot remember if I recreated it myself.) If I can recreate the issue in an instance of Pattern Lab, I then create a branch off of core with as reduced a test case as possible as fixtures, and attempt to create a failing unit test off of it. This is roughly what I did for #663

Let me know here or on gitter if that makes sense.

@raphaelokon
Copy link
Contributor

@bmuenzenmeyer Yeah, sorry. I was asking myself if any of the list items tests already reflect the OPs issue.

@bmuenzenmeyer
Copy link
Member

@raphaelokon np - I think that this issue exists it proof that the existing tests are not adequate. you will see some unit test coverage however inside list_item_hunter_tests.js

@raphaelokon
Copy link
Contributor

@bmuenzenmeyer Aye. I pinged you on Gitter

raphaelokon added a commit that referenced this issue May 23, 2017
@bmuenzenmeyer
Copy link
Member

I've manually recreated this issue too

image

Now to look into getting that failing unit test in place... will check out what you have Raphael

@raphaelokon
Copy link
Contributor

Cheers Brian. If you could amend the test case – I'd take it from there :-)

bmuenzenmeyer added a commit that referenced this issue May 24, 2017
@bmuenzenmeyer
Copy link
Member

@raphaelokon I will annotate that commit quick.

raphaelokon added a commit that referenced this issue Jun 7, 2017
@bmuenzenmeyer
Copy link
Member

this is still broken for me when running against actual files.

image

and i have the following failed unit tests

  400 passing (15s)
  4 failing

  1) test/parameter_hunter_tests.js parameter hunter finds partials with their own parameters and renders them too should be equal:

      Error: should be equal
      + expected - actual

      -<b>c</b><CR>
      -<b>b</b><CR>
      -<i>b!</i><CR>
      -<b>a</b><CR>
      -<i>a!</i><CR>
      -<CR>
      -<CR>
      +<b>c</b>
      +<b>b</b>
      +<i>b!</i>
      +<b>a</b>
      +<i>a!</i>
      +
      +

      at test/parameter_hunter_tests.js:115:8
      at Object.<anonymous> (test/parameter_hunter_tests.js:77:5)
      at run (bootstrap_node.js:394:7)
      at startup (bootstrap_node.js:149:9)
      at bootstrap_node.js:509:3

  2) test/pattern_graph_tests.js PatternGraph.fromJson() - Loading a graph from JSON should be equivalent:

      Error: should be equivalent
      + expected - actual

      -1496944867613
      +1337

      at tap.test (test/pattern_graph_tests.js:43:8)
      at Object.<anonymous> (test/pattern_graph_tests.js:41:5)
      at run (bootstrap_node.js:394:7)
      at startup (bootstrap_node.js:149:9)
      at bootstrap_node.js:509:3

  3) test/pattern_graph_tests.js PatternGraph.fromJson() - Loading a graph from JSON should be equivalent:

      Error: should be equivalent
      + expected - actual

      -[]
      +[
      +  "atom-foo"
      +  "molecule-foo"
      +]

      at tap.test (test/pattern_graph_tests.js:44:8)
      at Object.<anonymous> (test/pattern_graph_tests.js:41:5)
      at run (bootstrap_node.js:394:7)
      at startup (bootstrap_node.js:149:9)
      at bootstrap_node.js:509:3

  4) test/pattern_graph_tests.js PatternGraph.fromJson() - Loading a graph from JSON should be equivalent:

      Error: should be equivalent
      + expected - actual

      -[]
      +[
      +  {
      +    "v": "molecule-foo"
      +    "w": "atom-foo"
      +  }
      +]

      at tap.test (test/pattern_graph_tests.js:45:8)
      at Object.<anonymous> (test/pattern_graph_tests.js:41:5)
      at run (bootstrap_node.js:394:7)
      at startup (bootstrap_node.js:149:9)
      at bootstrap_node.js:509:3

@raphaelokon
Copy link
Contributor

raphaelokon commented Jun 8, 2017

Hmm … those tests are passing fine for me. I'll look into it …

The failing tests (2-4) are because /test/public/* folder got excluded.

@raphaelokon
Copy link
Contributor

Seems like we are excluding the public folder but trying to re-add the testDependencyGraph.json
https://github.com/pattern-lab/patternlab-node/blob/master/.gitignore#L10-L12

@raphaelokon
Copy link
Contributor

@bmuenzenmeyer Can we have a look into this one as well?

@stale
Copy link

stale bot commented Oct 2, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the needs response label Oct 2, 2017
@stale stale bot closed this as completed Oct 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants