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 angle brackets with docs-demo component #342

Closed
wants to merge 1 commit into from

Conversation

noslouch
Copy link

The following snippet wasn't getting picked up by the original regex:

<DocsDemo as |demo|>
  <demo.example @name='button-example.hbs'>
    <MyButton>
      Click Me
    </MyButton>
  </demo.example>

  <demo.snippet @name='button-example.hbs'/>
</DocsDemo>

This shouldn't break existing classic component invocation of docs-demo.

The following snippet wasn't getting picked up by the original regex:
```hbs
<DocsDemo as |demo|>
  <demo.example @name='button-example.hbs'>
    <MyButton>
      Click Me
    </MyButton>
  </demo.example>

  <demo.snippet @name='button-example.hbs'/>
</DocsDemo>
```

This shouldn't break existing classic component invocation of `docs-demo`.
Copy link
Contributor

@dfreeman dfreeman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @noslouch! Would you mind including a simple test for this as well?

begin: /{{#(?:docs-snippet|demo.example)\sname=(?:"|')(\S+)(?:"|')/,
end: /{{\/(?:docs-snippet|demo.example)}}/,
begin: /(?:{{#|<)(?:docs-snippet|demo.example)\s@?name=(?:"|')(\S+)(?:"|')/,
end: /(?:{{|<)\/(?:docs-snippet|demo.example)(?:}}|>)/,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this to work for standalone snippets, we'd need to accept DocsSnippet here as well, wouldn't we?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yeah that’s right. Good catch.

@RobbieTheWagner
Copy link
Member

@noslouch @dfreeman where are we on this?

@noslouch
Copy link
Author

noslouch commented Aug 1, 2019

Nowhere! I’m not sure where to start with a test for this 😞

@dfreeman
Copy link
Contributor

dfreeman commented Aug 1, 2019

@noslouch I think what I'd do is add a route in the testing addon (under test-apps/new-addon) that has snippets in each of the formats we want to support and then write an acceptance test that navigates to that page and verifies the expected content shows up for each.

@RobbieTheWagner
Copy link
Member

@noslouch do you think you could maybe add the tests @dfreeman described?

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