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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

5604 better avatar error handling #5638

Merged

Conversation

@Faldrian
Copy link
Contributor

commented Feb 8, 2015

This is a fix for #5604.

The solution was already in the code, but at the wrong place. When the code was executed at it's original position, the views were not yet rendered - so there were no elements to attach event handlers to.
I moved the code which adds event handler to the general view-render method... so it will be executed after each view is rendered (and only for the newly rendered view, to avoid doubling of event listeners).

I'm not quite happy with adding such a minor code-fixing-thing to THE MAIN VIEW-RENDER method... if you know what I mean 馃槒 . It seems a bit misplaced or can lead to people adding many more general fix things there...

Another solution could be to add the line views.js:51 to a postRenderTemplate() in every view with avatars in its templates. This would mean the code would only be executed when it is needed, making the whole process a bit faster when there are no avatars in a view ... but the implementing person of a new view must know about this and add the method to the new view (making the implementing a bit more troublesome).

@jaywink

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2015

IMHO this fix in the main renderer is well justified - we use avatars throughout the site and thus it is actually a good place to put the fix imho. At least for now since we're dealing with regression which has to be fixed for 0.5 to go out.

So 馃憤 from me at least. Triggered rebuild of random failed test.

@jaywink jaywink added this to the next-major milestone Feb 8, 2015

@jaywink

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2015

Thanks @Faldrian !

jaywink added a commit that referenced this pull request Feb 8, 2015

@jaywink jaywink merged commit e6d38db into diaspora:develop Feb 8, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@jaywink

This comment has been minimized.

Copy link
Contributor

commented Feb 9, 2015

One place seems to not work still - the notifications drop down:

@Faldrian

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2015

I'll have a look at that later this week, thanks for testing.
(Probably that view does not extend app.views.Base )

@Faldrian Faldrian deleted the Faldrian:5604-better-avatar-error-handling branch Feb 9, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can鈥檛 perform that action at this time.