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

Convert to angle bracket notation in version 3.7 #357

Merged
merged 9 commits into from
Jan 24, 2019

Conversation

cah-danmonroe
Copy link
Contributor

Here is the base of the angle bracket notation conversion. There are a few things that will need addressed:

Need a new image for /images/ember-core-concepts/ember-core-concepts.png to convert

tutorial/simple-component and components/defining-a-component :
Update: "A dash is required in every component name to avoid conflicting with a possible HTML element, so rental-listing is acceptable but rental isn't."

Are these correct?

{{#each this.model as |post|}}
  <BlogPost @post.title @post.body/>
{{/each}}
<MyComponent @people={{array
    'Tom Dade'
    'Yehuda Katz'
    this.myOtherPerson}}
 />

@theroncross
Copy link
Contributor

Most of the other examples I've seen included the xhtml space before closing (...@post.body />). Is that a thing we care to care about?

@jenweber
Copy link
Contributor

I'm going to try converting the Super Rentals app because I need to learn how to do this. Will PR it and @ @DanMonroe for review

@jenweber
Copy link
Contributor

P.S. you are the real MVP for tackling this! Thank you!

@jenweber
Copy link
Contributor

Super Rentals conversion tracked in ember-learn/super-rentals#114

@jenweber
Copy link
Contributor

<MyComponent @people={{array
    'Tom Dade'
    'Yehuda Katz'
    this.myOtherPerson}}
 />

To answer your question, that is correct! I tested it.

@jenweber
Copy link
Contributor

{{#each this.model as |post|}}
  <BlogPost @post.title @post.body/>
{{/each}}

From the RFC:

Positional arguments ({{foo-bar "first" "second"}}) are not supported.

I tested this and found that the statement is accurate. We should show regular components for the positional params example and explain that you can't use Angle Brackets, and recommend a refactor away from positional params

@jenweber
Copy link
Contributor

Thanks everyone!!! I will review tonight and get at least 1 more person to look it over.

Next steps are to fix the test failures. We can also add in a few call outs linking to earlier guides versions, but that can come after a merge to master

Copy link
Member

@mansona mansona left a comment

Choose a reason for hiding this comment

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

2 Minor comments that I'm pretty sure don't affect the functionality of the examples 👍 I'm happy for them to make it through and we can decide to fix them later if we feel like it 😂

```handlebars {data-filename=app/templates/index.hbs}
{{#each this.model as |post|}}
{{!-- either foo-component or bar-component --}}
{{#let (component (concat this.componentName)) as |Post|}}
Copy link
Member

Choose a reason for hiding this comment

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

Is this concat left over from something? shouldn't it be:

Suggested change
{{#let (component (concat this.componentName)) as |Post|}}
{{#let (component this.componentName) as |Post|}}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -92,7 +92,7 @@ module('Component | counter', function(hooks) {
test('it should count clicks', async function(assert) {
this.set('value', 0);

await render(hbs`{{x-counter value=value onUpdate=( … )}}`);
await render(hbs`<XCounter @value={{this.value}} @onUpdate={{( … )}} />`);
Copy link
Member

Choose a reason for hiding this comment

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

Do we still want to use x-counter here? considering we don't need it any more? i.e. we can have single word components like <Counter>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has already been converted. Are you still seeing it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still seeing XCounter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the X

@mansona
Copy link
Member

mansona commented Jan 22, 2019

Regarding the CI Failures, I have marked Travis as required as it's the only thing that needs to be fixed before we merge this 👍

The Netlify stuff can be ignored for now as it's related to #365 👍

@@ -0,0 +1,72 @@
The [Angle Bracket Syntax](https://github.com/emberjs/rfcs/blob/master/text/0311-angle-bracket-invocation.md) is an alternative style of invoking components in templates. The difference between the Angle Bracket Syntax and the Classic Invocation Syntax is purely syntactical and does not affect the semantics of invoking a component.
Copy link
Contributor

Choose a reason for hiding this comment

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

@chrisrng Thanks for writing this! I would like to maybe put this info in a "conversion guide" type section that is outside of templates. Maybe something like reference/syntax-conversion-guide that could be multi-purpose to other types of syntax. URLs are forever. Since we are teaching angle brackets as the default, we want to avoid a URL structure that makes them seem "new" if that makes any sense. What do you think about moving it under a different topic? Then in places where we use Angle Bracket syntax, we link to this explanation.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good, makes sense!
I've opened a PR to move them there:
cah-danmonroe#2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved.

@jenweber
Copy link
Contributor

This work is so so so good, thanks everyone ❤️

I think our only blocker to merging is my request to move angle brackets under some different table of contents URL heading. In CLI docs, the equivalent is "Appendix" so maybe we should follow the same structure. See here. @locks any thoughts?

Callouts linking to the older guides/the explanation can follow in a later PR, after merging this to master, since guides-source is not continuous deployment (yet 😉 )

@jenweber
Copy link
Contributor

Ran this locally and everything looks great! Table of Contents renders as expected, tests pass, etc.

Thanks everyone for your work!!! Merged.

@jenweber jenweber merged commit be4e09c into ember-learn:master Jan 24, 2019
jaredgalanis pushed a commit to jaredgalanis/guides-source that referenced this pull request Feb 10, 2019
* Convert to angle bracket notation in version 3.7

* fixing travis

* updates from review comments

* updates from review comments

* Add explanation on Angle Bracket Syntax vs Classical Invocation

* Removed unnecessary concat in example

* 37 angle brackets - Fixed spelling

* Refactored x-counter to counter

* Moved angle bracket conversion guide to new reference section
cspanring pushed a commit to cspanring/guides-source that referenced this pull request Apr 12, 2019
* Convert to angle bracket notation in version 3.7

* fixing travis

* updates from review comments

* updates from review comments

* Add explanation on Angle Bracket Syntax vs Classical Invocation

* Removed unnecessary concat in example

* 37 angle brackets - Fixed spelling

* Refactored x-counter to counter

* Moved angle bracket conversion guide to new reference section
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants