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

Migrate Notification initializer to Pack tag #18998

Closed
lboogie04 opened this issue Jan 24, 2023 · 6 comments · Fixed by #19124
Closed

Migrate Notification initializer to Pack tag #18998

lboogie04 opened this issue Jan 24, 2023 · 6 comments · Fixed by #19124
Assignees

Comments

@lboogie04
Copy link
Contributor

In an effort to migrate away from asset pipeline, the current initializers need to be migrated to pack tags via Webpacker and initialized in the perspective view/page.

The initNotifications() should be migrated to a pack tag and added to the perspective views in which this code should be initialized.

@lboogie04 lboogie04 added this to the Initializer Migrations milestone Jan 24, 2023
@github-actions
Copy link
Contributor

Thanks for the issue, we will take it into consideration! Our team of engineers is busy working on many types of features, please give us time to get back to you.

Feature requests that require more discussion may be closed. Read more about our feature request process on forem.dev.

To our amazing contributors: issues labeled bug are always up for grabs, but for feature requests, please wait until we add a ready for dev before starting to work on it.

To claim an issue to work on, please leave a comment. If you've claimed the issue and need help, please ping @forem-team. The OSS Community Manager or the engineers on OSS rotation will follow up.

For full info on how to contribute, please check out our contributors guide.

@Ridhwana
Copy link
Contributor

Ridhwana commented Feb 2, 2023

Correct me if I'm wrong but from my understanding these pack files are not tested. If they aren't, would we want to add some tests for them as we migrate them? What do we think the effort vs reward for these would be? cc @mirie @lboogie04

@Ridhwana
Copy link
Contributor

Ridhwana commented Feb 2, 2023

Also curious to get @maestromac's perspective on this as he works on increasing test coverage.

@mirie
Copy link
Contributor

mirie commented Feb 3, 2023

@lboogie04 / @maestromac what are your thoughts on this? I added some buffer to include testing in the estimate

@maestromac
Copy link
Member

Yes, I think this is a ripe opportunity to also add test along with the migration and I apologize for the additional work this may put on @lboogie04

@lboogie04
Copy link
Contributor Author

@Ridhwana Most definitely!

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 a pull request may close this issue.

4 participants