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

Delete news item #213

Merged
merged 2 commits into from
Feb 19, 2014
Merged

Delete news item #213

merged 2 commits into from
Feb 19, 2014

Conversation

mreinhardt
Copy link
Contributor

(Refs Issue #100)

  • User can delete their own news item (any child comments deleted as well)
  • Make delete button a mixin
  • Add confirmation modal to both news item and comment delete

@megamattron
Copy link
Member

Great, definitely need this feature. Would you mind a quick rebase? There's a conflict I'm not sure about.

(Refs Issue larvalabs#100)
* User can delete their own news item and all child comments
* Make delete button a mixin
* Add confirmation modal to both news item and comment delete
@mreinhardt
Copy link
Contributor Author

@megamattron Rebased and resolved.

@treygriffith
Copy link
Collaborator

It seems kind of strange to delete other users' comments on a news item when the news item is deleted, but it also seems strange to keep them. Any thoughts on potential issues with this?

@mreinhardt
Copy link
Contributor Author

Thought about this, but I kind of like a scorched earth policy. If someone wants to change this in a later pull request they're obviously certainly welcome to. 😸

@treygriffith
Copy link
Collaborator

Haha fair enough. I guess if it becomes a problem we can change it.

megamattron added a commit that referenced this pull request Feb 19, 2014
@megamattron megamattron merged commit f9c3bb0 into larvalabs:master Feb 19, 2014
@megamattron
Copy link
Member

Ok thanks!

@mreinhardt mreinhardt mentioned this pull request Feb 20, 2014
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

3 participants