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

Can we abstract away the idea of working with regions... #2148

Open
jasonLaster opened this issue Dec 18, 2014 · 5 comments
Open

Can we abstract away the idea of working with regions... #2148

jasonLaster opened this issue Dec 18, 2014 · 5 comments
Milestone

Comments

@jasonLaster
Copy link
Member

@jmeas kicked off an interesting discussion in #2145 w/r/t de-emphasizing regions.

@paulfalgout, @tbranyen weighed in with good points as well.

@jamesplease
Copy link
Member

It should be fairly easy to move away from regions. To start, we would first need to provide all of the functionality that regions provide on the view itself. Once we make this available, I'm confident that people will find it preferable to work with. From there, we can add migration guides / potentially automate some of it, let it simmer for a few months, then finally remove the old convention for working with regions.

I'll cover two big reasons why people interact with views directly:

  1. Multiregions. The reason to use Multiregions is that you want to show n views as siblings without the wrapping element. Sort of a manually-managed list.

This can be solved by making the region function as a placeholder. When a view is shown in a region, the default behavior should be that the region's element is plucked out of the DOM and replaced with the view's element. There are a slew of other benefits we get from this, as well. Minionette and Angular implement a placeholder-swap-and-replace algorithm, so we can look at those for inspiration.

LayoutManager solves the wrapping problem in another way: it makes the view's elements optional. @tbranyen, correct me if I'm wrong, but you don't swap the view in for the matched element that you're showing? In other words, after I've called myView.setView('.my-selector', new MyView()), is .my-selector still around?

If the answer is yes, then I prefer swapping out the region's element vs. making the view's element optional. Both cases solve for the unnecessary wrapper in a large number of use cases.

Another concern are dynamic regions. This is easily solved by letting folks quickly instantiate regions with selectors (or maybe even elements). When the UI updates, get a reference to the newly-added element and show a view in it. And that's that. Dynamic regions. Creating dynamic regions in a layoutview is pretty clunky right now.

  1. Animated regions. This can be solved if we use an API/implementation similar to ngAnimate / React, which I think we should.

The next step would be to remove all events from regions, and push these up to the view itself. Instead of myView.listenTo(this.getRegion('main'), 'empty');, you would do this.on('empty:region'), or whatever. The name / selector of the region would be passed as the first arg. In this way, we lose no functionality from the perspective of the events, and the API now now describes regions as a part of the view, and not a separate thing.

Lastly, we would remove all API methods that directly return the region instance. You would always go through the view's API.

Some other benefits of swappable regions are that we can get rid of the CompositeView class. The only reason CompositeView exists (ignoring the terrible recursive behavior) is that you need it to show tables. If we swap out the region's element, you can get the CompositeView effect with just a Marionette.LayoutView (or, in this system, the plain old Marionette.View). Minionette manages to pull off a CompositeView by allowing the CollectionView to have a template. I haven't looked at the API to see how that works, but it's worth looking into, I think.

I know a number of people treat regions as quasi-view objects, listening to their events, overriding them, and so on, but I think that this is probably something we should move away from. We can accomplish the exact same things without this extra concept, with fewer code and a cleaner, more succinct API. I'm strongly in favor of us moving in the direction of making regions less of a 'feature' of Marionette.

It'll be tough, but in the end, I think we would all be happy with the change, and Marionette would be a far cleaner library.

@samccone
Copy link
Member

Animated regions. This can be solved if we use an API/implementation similar to ngAnimate / React, which I think we should.

mind expanding on this

@jamesplease
Copy link
Member

No time now, but I'll be working on it sometime soon-ish.

@samccone
Copy link
Member

ok the only reason I ask is because that is the hardest problem with a regions "rewrite". By that i mean async destroy of regions and their contents.

@stephanebachelier
Copy link
Contributor

@samccone for my own projects I've create an AnimatedRegion, which basically add some class on the view el to animate the view inside the region.

My objective are quite simple:

  • only CSS animation
  • animate methods (animateIn, animateOut): to add the animation logic, the default being classes added to the view, but you can override this to change the whole animation logic,
  • triggers onAnimateIn, onAnimateOut when animation is complete.

It's still a work in progress while being used in some projects so expect some changes.
Just a few notes:

  • I need to add documentation, tests and examples.
  • for now I've not used jQuery and I will add support for this as an opt-in. It's already done but not pushed
  • I used a mutation observer to launch the animation only when the view is inside the DOM. I don't like onDomRefresh logic so MutationObserver was the best option IMHO being async.
  • I use another library of mine motioncontrol which I've created to detect CSS animation or transition ending. It's based on others works and huge testing.

As I've already said before, this is still a working in progress, I would set beta, as I already use this in production, or next-to-be production code. Expect a few changes in the coming days.

@ahumphreys87 ahumphreys87 added this to the v3.0.0 milestone Apr 12, 2015
@jasonLaster jasonLaster modified the milestones: v3.0.0, v3.x Aug 18, 2015
@paulfalgout paulfalgout modified the milestones: v3.x, v4.x Aug 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants