Skip to content

Conversation

@mcfiredrill
Copy link
Contributor

Changed the templates from .hbs to .gjs, and added relevant imports.

I didn't add the import or when it looked like the example was only showing part of a template.

Changed the templates from .hbs to .gjs, and added relevant imports.

I didn't add the import or <template> when it looked like the example was only showing part of a template.
@netlify
Copy link

netlify bot commented Oct 23, 2025

Deploy Preview for ember-guides ready!

Name Link
🔨 Latest commit 8cda408
🔍 Latest deploy log https://app.netlify.com/projects/ember-guides/deploys/68fe547583e2c60008149057
😎 Deploy Preview https://deploy-preview-2175--ember-guides.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

<h1>Latest Comments</h1>
<ul>
{{#each this.latestComments as |comment|}}

Choose a reason for hiding this comment

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

Does this.latestComments still work in route templates?
Shouldn't this be @controller.latestComments?

Copy link
Member

Choose a reason for hiding this comment

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

very good point 👍 yes that would need to be @controller

Choose a reason for hiding this comment

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

In that case, it probably applies to the other references to this as well so those also should be updated

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I was just going to suggest that you look at all the this. references in the newly converted gjs files. Some may come from the model and some from a controller.

Additionally, you will need to add import statements for everything you use in the template, like array and excerpt used below.

Copy link
Member

Choose a reason for hiding this comment

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

well just to be clear here, until you explicitly added the file name this template wasn't technically wrong. You had "plausable denyability" that you were dealing with a component or something (although there was no backing class so the argument is very weak).

If you want to be 100% correct there should be a backing class that shows some latestComments and you can see what the template is referring to without having to guess that there is a controller that happens to have the latestComments field on it.

I hope all that makes sense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback! I think I know what to do, I'm working on addressing this now then I'll ask for more feedback.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it be better to write a backing class in the template or assume that latestComments came from a controller and just use @controller.latestComments ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I hate controllers, so I'm a fan of using the stateful component pattern instead of controller lol

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I guess we don't necessarily need to indrectly encourage controller use here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a backing class version, although we had some discussion about longer term direction in the discord. https://discord.com/channels/480462759797063690/480777444203429888/1432054589661778003

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.

thanks for this! can you update the language on the code blocks that you changed too?

@mcfiredrill
Copy link
Contributor Author

@mansona done!

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.

5 participants