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

Implement setRenderer destructive option #3556

Closed
wants to merge 1 commit into from

Conversation

blikblum
Copy link
Collaborator

Proposed changes

  • when passing an options hash to setRenderer with destructive: false, do not reinit regions on view re-render

Link to the issue: #3427

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling a986fdd on blikblum:setrenderer-destructive into ba77bc0 on marionettejs:next.

@paulfalgout
Copy link
Member

I'd support this for an experimental API. Looking forward to v4.x and v5 for region improvements and I don't want to box ourselves in.

@blikblum
Copy link
Collaborator Author

I'm ok with marking as an experimental API.
In fact, i think the renderer api should be extended to handle other things like dom:refresh and dom:remove events that does not make sense with non destructive renderers

Copy link
Member

@scott-w scott-w left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a fan overall, I just think the naming could be improved (see my comment).

this.prototype._renderHtml = renderer;
if (options && options.destructive === false) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the feature could be good, though I don't like the name.

Maybe just call this reInitializeRegion or maybe preserveRegion so the flag check can be options.preserveRegion.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine changing the name but should not be region specific. This flag may be needed for other things than reinitialize region like for example adding an onReady event which would have a different behavior from destructive and non destructive regions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about preventDestroy? It's used in stuff like region.show and it feels like it does a similar job.

@paulfalgout
Copy link
Member

Something like this may make it to https://github.com/marionettejs/marionette

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants