allowMissing option permits non-DOM-backed regions

[duncanbeevers]

We use a Layout parent class to capture behavior common to a number of subclass views.

There are behaviors that drive regions which exist in some of the subclasses and not in others. Prior to #1024 / #1050 these missing regions would simply act as no-ops in these subclass views, but now they throw errors.

In order to accommodate this use-case, I added an allowMissing option to be passed in through show, which suppresses the error-throwing behavior in _ensureElement, and turns appendHtml into a no-op in the case of a missing this.el`.

[jasonLaster]

Hey @duncanbeevers - what cool behaviors have you built?

[duncanbeevers]

We use this type of control for a media library manager / player. We use these subclasses to implement different views of the library (posters, list, and details views)

In this case, if the media is currently playing, some representations include a little progress meter, while others don't.

[jasonLaster]

oh fun. media players are one of those canonical widgets. I'd love to see a more flushed out breakdown of how you designed the player and the media types.

I think you're right that we should have a solution for flagging off the empty region error.

1. user sub-classes region and overrides two functions
2. Mn adds allowMissing flag show
3. Mn adds allowMissing flag to Region class. and then user sub-classes Region or passes that option in as a region option when setting up the Region.

My preference is for solution 3 because this seems like something you want to disable on a class-by-class basis. What do you think?

[duncanbeevers]

The behavior definitely makes sense to configure on a class-by-class basis, but unfortunately we exclusively set up regions in LayoutViews, and so do no manual Region instantiation. I guess this means we'd have to create our own RegionManager that dispensed these subclassed Regions.

[jasonLaster]

@duncanbeevers I think we could do this:

new Marionette.LayoutView({
 regions: {
   "cat": ".doge",
   "wow": {
     allowMissing: false,
     regionClass: Coin
   }
 }
})

[duncanbeevers]

I've pushed a slightly different PR based on your feedback.

allowMissing is now part of a Region's options (rather than a explicit property of the Region)

This allows us to change our region definitions to this form.

regions: {
  progressRegion: {
    selector: '.progress-region',
    allowMissing: true
  }
}

[jasonLaster]

This looks good to me. Mind updating the docs as well, which will mean updating the docs/region.md file and the api/region.jsdoc file. We hope to roll out the new api in the neartime future.

[jamesplease]

Let's hold off on this. I'm not sure if I think that allowMissing is a good idea.

[duncanbeevers]

@jmeas Do you have any concrete feedback? I'd love to discuss the right way to go about handling this situation, whether it's making a publicly-overridable region method like onMissingElement, making an instance property on region at class-definition time, defining an instance-specific option as in this case, or some other solution.

As it stands, _ensureElement is doing at least two jobs, one of which I would like to suppress.

[jamesplease]

I didn't mean to hold this up for so long. The rest of the team can make a decision on which to merge – I doubt I'll have time to think more on this issue. Thanks for the great PRs, @duncanbeevers!

[jasonLaster]

Hey @duncanbeevers, thanks for offering #2025 as an alternative approach. We would definitely like to make it possible to specify that a region can infact have a missing element.

I'd like to hear which approach you prefer. I've spoken with @jmeas and either one is fine with us. Personally, I'm inclined to go with this approach because it is easier to specify in the Region setup that the region will not insist on having an element.

regions: {
  progressRegion: {
    selector: '.progress-region',
    allowMissing: true
  }
}

One side thought, maybe allowMissingEl would be a lit bit clearer.

[duncanbeevers]

I modified this PR a bit to ensure views provided to non-backed regions aren't rendered.

The allowMissing option is now allowMissingEl

[duncanbeevers]

I'm not sure what's up with the TravisCI failures. The tests are green on my machine and the failures appear unrelated to the changes I've proposed here.

[jasonLaster]

Hmm, @duncanbeevers - it looks like the PR wants to merge onto master. mind opening a new PR against minor?

TravisCI failures

not sure why Behavior tests would start failing. hopefully the new PR will fix that....

[duncanbeevers]

@jasonLaster Reopened against minor as #2028