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

Ameliorate forum-post-new reactivity #657

Merged

Conversation

DrumsnChocolate
Copy link
Contributor

@DrumsnChocolate DrumsnChocolate commented Nov 19, 2022

ref #641
rewrote to glimmer component, because I understand them better, and it allows me to use the this.args namespace (see https://guides.emberjs.com/release/upgrading/current-edition/glimmer-components/#toc_benefits-of-glimmer-components)
note that args are immutable in glimmer components, which requires an extra function to be passed as an argument. Via this function onChangeContent we can pass the new value up to the containing controller, in this case posts/index
(If you're familiar with Vue.js then this pattern should look familiar.)

This would be fixed when #539 is finished, but I thought it wouldn't hurt to fix this since creating forum post is still broken right now

…e new data binding pattern that is standard for glimmer
@@ -1,5 +1,5 @@
<p class='card-text'>
<MdEditor @content={{content}} @textareaId='newForumPost' />
<MdEditor @content={{this.content}} @textareaId='newForumPost' />
Copy link
Contributor Author

@DrumsnChocolate DrumsnChocolate Nov 19, 2022

Choose a reason for hiding this comment

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

note that this no longer refers to the content argument, but rather to the content value that comes from the controller. In this situation, that content property on the controller comes in the form of a getter.
If we wanted to still refer to the content argument, we would need to write @content rather than this.content

In glimmer components, this refers to the component itself, the component 'controller' so to speak. This is similar to how in octane route templates, this refers to the controller of the route.

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 also find that this nicely demonstrates how glimmer components will clarify code in general

@DrumsnChocolate DrumsnChocolate changed the title Fix forum post new reactivity Fix forum-post-new reactivity Nov 19, 2022
@DrumsnChocolate
Copy link
Contributor Author

DrumsnChocolate commented Nov 19, 2022

wait, this is already being taken care of in #567

nevertheless, the extra work here is useful, (and I came to basically the same solution but better, I'd say that is a great sign)

@DrumsnChocolate DrumsnChocolate changed the base branch from staging to fix/forum-post-text-reset November 19, 2022 21:53
@DrumsnChocolate DrumsnChocolate changed the title Fix forum-post-new reactivity Ameliorate forum-post-new reactivity Nov 19, 2022
@DrumsnChocolate DrumsnChocolate merged commit 6df538e into fix/forum-post-text-reset Nov 19, 2022
@DrumsnChocolate DrumsnChocolate deleted the fix/641-form_post_new_reset branch November 19, 2022 22:01
@guidojw
Copy link
Member

guidojw commented Nov 27, 2022

Ameliowhat

@DrumsnChocolate
Copy link
Contributor Author

Ameliowhat

https://www.merriam-webster.com/dictionary/ameliorate
image

@wilco375
Copy link
Contributor

Ameliowhat

Haha exactly my thought when I read this PR

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.

forum-post-new component does not clear contents upon cancel
3 participants