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

Add topLevelCommentId field to comments. #943

Merged
merged 2 commits into from May 6, 2015
Merged

Add topLevelCommentId field to comments. #943

merged 2 commits into from May 6, 2015

Conversation

cerupcat
Copy link
Contributor

@cerupcat cerupcat commented May 5, 2015

This allows childComments to know what the top most parent comment is by adding a topLevelCommentId to each childComment.

…lows childComments to know what the top most parent comment is.
@cerupcat cerupcat changed the title Add topLevelCommentId field to comments along with migration. Add topLevelCommentId field to comments. May 6, 2015
@SachaG
Copy link
Contributor

SachaG commented May 6, 2015

Awesome, thanks!

SachaG added a commit that referenced this pull request May 6, 2015
Add topLevelCommentId field to comments.
@SachaG SachaG merged commit fa7e057 into VulcanJS:devel May 6, 2015
@cerupcat
Copy link
Contributor Author

cerupcat commented May 6, 2015

Great - thanks SachaG. You're fast! :)

@SachaG
Copy link
Contributor

SachaG commented May 19, 2015

Taking another look at this, I don't think we can keep that migration. There's three nested forEach loops in there, and I'm not sure if that's a good idea for sites like Crater that already have a lot of comments…

Maybe we just need to leave current data alone and assume the topLevelCommentId property might not always exist?

@cerupcat
Copy link
Contributor Author

Yeah, I mean, if people don't need this for old comments threads, then it shouldn't be a big deal to not have a migration. I was't sure how to do the migration any simpler.

@SachaG
Copy link
Contributor

SachaG commented May 19, 2015

Yeah I don't know how you could do it simpler either. It's more about doing it with https://github.com/percolatestudio/meteor-migrations or some similar system that can account for how long it'll take. Telescope's current migrations system is pretty bare-bones, so it can mess up with the deploy process.

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