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

Getting error with notification.add() #162

Closed
mapsam opened this issue Jul 11, 2015 · 4 comments
Closed

Getting error with notification.add() #162

mapsam opened this issue Jul 11, 2015 · 4 comments
Labels

Comments

@mapsam
Copy link
Member

mapsam commented Jul 11, 2015

Uncaught TypeError: Cannot read property 'add' of undefined

Coming from this line: https://github.com/cugos/dropchop/blob/geo-menu-updates/src/js/controller/AppController.js#L118 - @alukach does this have to do with the notification changes from #154 ?

@mapsam mapsam added the bug label Jul 11, 2015
@alukach
Copy link
Member

alukach commented Jul 11, 2015

So based on that link, I'm guessing that this is from the geo-menu-updates branch?

Looking at the controller source code, it appears that the this.notification = new L.Dropchop.Notifications(); declaration was removed, likely due to a bad merge.

This appears to have occurred during the 73ecce9, although the diff doesn't actually show it being removed (I think I've seen this before, it's a weird thing that can happen with merging where the diffs don't represent changes made in the merge). Take a look at the the code from commit 73ecce9 compared to the commit before (fd12ff0018).

EDIT: Scratch everything I just said. I now see the this.notification declaration on line 74.

@mapsam
Copy link
Member Author

mapsam commented Jul 11, 2015

Should it be added back? Definitely may have scraped it accidentally when merging master into geo-menu-updates.

UPDATE: Adding it back fixes, but since it doesn't exist in master I feel like something may still be amiss.

@alukach
Copy link
Member

alukach commented Jul 11, 2015

Yeah, removing it in this commit caused the issue.

It does exist on master: https://github.com/cugos/dropchop/blob/master/src/js/controller/AppController.js#L24

So all seems good, right? Just need to re-add it and this brach should be working as needed.

@mapsam
Copy link
Member Author

mapsam commented Jul 12, 2015

Thanks for finding the commit to blame. Must have been moving through that merge too quickly 😝

@mapsam mapsam closed this as completed Jul 12, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants