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

Update class field description component to glimmer #810

Merged
merged 3 commits into from
Jul 24, 2022
Merged

Update class field description component to glimmer #810

merged 3 commits into from
Jul 24, 2022

Conversation

geneukum
Copy link
Contributor

@geneukum geneukum commented Jul 22, 2022

Let's update the ClassFieldDescription component to extend glimmer/component instead of ember/component. Glimmer components offer a bunch of various advantages over classic components and are the direction that ember is
heading in general.

While we're here, let's prefer single quotes to double quotes throughout our template file.

Finally, let's make sure that we're using angle bracket syntax rather than curly brace syntax to invoke the component in our tests as this cleans up some deprecations logged when the tests are run.

This should resolve the app/component/class-field-description.js section of #805.

Let's update the ClassFieldDescription component to extend glimmer/component
instead of ember/component. Glimmer components offer a bunch of various
advantages over classic components and are the direction that ember is
heading in general.
Let's prefer single quotes to double quotes throughout our template file.
Let's prefer angle bracket syntax to curly syntax when invoking the
ClassFieldDescription component in our integration tests. This will fix
deprecation warnings about using the latter, which make the test results a bit
harder to decipher.
@jenweber
Copy link
Contributor

Hi @geneukum! Thank you so much for these improvements. It will take me a bit of time to go through them and review. I'll aim to do that one at a time over the next week or so. Modernizing this app is a big help, as we are about to try and add some more features to it over the next year to support TypeScript examples.

@jenweber jenweber merged commit 361f97c into ember-learn:master Jul 24, 2022
@geneukum
Copy link
Contributor Author

Hi @geneukum! Thank you so much for these improvements.

Absolutely! I'm happy to help however I can. 😄

It will take me a bit of time to go through them and review. I'll aim to do that one at a time over the next week or so.

That sounds great! Thanks so much for taking a look.

Modernizing this app is a big help, as we are about to try and add some more features to it over the next year to support TypeScript examples.

Speaking of TypeScript, do you think it makes sense to pull in ember-cli-typescript and start converting some of this stuff over in a follow-up issue?

@jenweber
Copy link
Contributor

Good question! Let me check in with the rest of the team. Last time the learning core team discussed adding TypeScript for the front-end apps we maintain, we decided not to do it. That was before there was a dedicated TS core team though.

The team was ok adding it to back end node apps like ember-jsonapi-docs. I’m currently rewriting that repo though so it’s not a good candidate either at this moment. When I get a little farther, I’d like to use TypeScript in it!

Please feel free to @ me if you have any questions about this app or other projects in Ember.

@geneukum geneukum deleted the update-class-field-description-to-glimmer branch July 25, 2022 22:42
@geneukum
Copy link
Contributor Author

geneukum commented Nov 8, 2022

hey @jenweber, I hope all is well! Is there any chance that we could land some of the other PRs from this set in the not too distant future?

I would be happy to go through whatever is remaining after a few more of them are merged. If I remember correctly, I was starting to get to the point where it seemed like they would conflict with one another.

Thanks again for all of your help!

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.

None yet

2 participants