Unnecessary line?

[matthew-andrews]

Hi @wilsonpage - we're discussing in a private issue about whether this line: https://github.com/ftlabs/fruitmachine-media/blob/master/media.js#L71 is necessary.

@rowanbeentje says:

Thinking about this a little more - on setup we still need this (because the media-query-state-change functions don't get triggered on setup, so this is still needed on initial setup). On teardown we don't need this because the element is likely to go away anyway; if it is going to get recycled - eg via a second setup call - the same helper should still be bound anyway so this is harmless.

I'm concerned that as I don't know the code that well that we might be missing something....

[wilsonpage]

I'd say this is required as otherwise the teardown for the currently active state wouldn't get run.

If this teardown function had logic like window.removeEventListener, it would never get run and you'd end up with zombie callbacks firing after the view had been destroyed.

Am I missing something...?

[rowanbeentje]

I suppose we do still need to call the run('teardown', { on: name });... @wilsonpage the big reason for removing this was the module.el.classList.remove(name);, so as every fruit module was torn down they amended their classes. This was causing quite a bit of layout thrashing...

So I suppose I should separate this into dom/event-type processing? We always want to tear down events - do we ever want to tear down the dom in this helper?

[wilsonpage]

Ahh, ok. I don't think it makes sense to remove the last active class then. I think those classes serve the same purpose as media queries, so I guess don't remove them when the view is fully torndown, only when moving between breakpoints.

[rowanbeentje]

Addressed by pull request #2. Thanks Wilson :)