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

Remove Vanna Controller from app and fix up notifications controller, views, scripts, specs #2714

Merged
merged 7 commits into from Jan 23, 2012

Conversation

stwf
Copy link
Contributor

@stwf stwf commented Jan 22, 2012

Here is the code I needed to remove the VannaController from D*. I did it from the unread notifications branch because that has some improvements. I didn't realize those commits would also be in here. Git newbie working!

Feel free let me know if there is a better way to do this... Or if I'm causing any problems. All the tests pass and what I could test works......

@sarahmei
Copy link
Member

<3 your branch name

@stwf
Copy link
Contributor Author

stwf commented Jan 23, 2012

In an alternate verse she and I are the best of friends!

@stwf
Copy link
Contributor Author

stwf commented Jan 23, 2012

Universe ugh

@dnsco
Copy link
Member

dnsco commented Jan 23, 2012

@sarahmei is there a problem with requiring the ApplicationHelper in application controller? do we have all helpers loaded by default? was that turned off?

@sarahmei
Copy link
Member

I thought we had all helpers turned on by default.

@maxwell
Copy link
Member

maxwell commented Jan 23, 2012

They should be. I think it was in the vanna controller because canna controller was not inherited from application controller.

@danielgrippi
Copy link
Member

if you axe that ApplicationHelper line, I'd say we're ready to pull it in, yes?

@stwf
Copy link
Contributor Author

stwf commented Jan 23, 2012

If I take that out I get undefined local variable or method `set_header_data' for <HomeController - tooooo long>:HomeController.. Which is the function that I moved out of ApplicationController, because a comment told me too, lol... Maybe that should go back in?

@maxwell
Copy link
Member

maxwell commented Jan 23, 2012

Yeah you want that in application controller, not sure who told you that :p

@stwf
Copy link
Contributor Author

stwf commented Jan 23, 2012

ok, its all in. Only took 2 commits! first master pull!

@danielgrippi
Copy link
Member

running rake on my machine right now. should be pulled in shortly!

danielgrippi added a commit that referenced this pull request Jan 23, 2012
Remove Vanna Controller from app and fix up notifications controller, views, scripts, specs
@danielgrippi danielgrippi merged commit 2c50de0 into diaspora:master Jan 23, 2012
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

5 participants