Created Sortable component with modifiers

[cah-brian-gantzler]

Do NOT Merge this PR. Possible improved syntax

I did not update the read.me, wasnt sure how you wanted to promote. Did write a migration guide.

Wanted to ask what groupModel was for, it doesnt appear to be used except for echoed back in onChange, there are better ways to do that now if thats all it is. Possibility of simplifying that?

Im sure theres some code to be cleaned up. Still making passes.

I created a new route in the dummy app to have the old and new displayed on two different routes, then just copied the acceptance tests. They were quite extensive and easily told me what wasnt working. To that end I didnt see any reason to have any component or modifier tests as it was already tested.

[ygongdev]

The groupModel is just an useful utility, allowing consumers to be flexible with the parent of the reordered models. There were some non-trivial use cases in v1, so I decided to keep it for v2 since there is no reason to remove it as it can only help you.

[cah-brian-gantzler]

I expected 3.4 and 3.8 to work. There must be something weird with a computed. Will look into it. I also dont have firefox installed locally.

[ygongdev]

This is quite a sizable change, although mostly additions. I'm not too familiar with the modifiers. I would feel more confident if we can have a review from someone who's more familiar with this. @lifeart maybe? 😃

[cah-brian-gantzler]

The groupModel is just an useful utility, allowing consumers to be flexible with the parent of the reordered models. There were some non-trivial use cases in v1, so I decided to keep it for v2 since there is no reason to remove it as it can only help you.

If Im right and the groupModel is not used anywhere except on the echo back of onChange, a better way to do this is

{{sortable-group model=model.items groupModel=model onChange=(action "update")}}

{{sortable-group model=model.items onChange=(action "update" model)}}

The above two statements should be equivalent. Except that the component doesnt need to track a variable it doesnt care about. Note: This currying is replaced by the new fn helper, when using @action on the update method. modern syntax would be

<SortableGroup @model={{this.model.items}} @onChange={{fn this.update this.model}}>

[cah-brian-gantzler]

Yes it is ALL new, there are no changes to the existing components. I still need to look into the computed failing 3.4 and 3.8. So this isnt yet ready to be merged anyway.

[cah-brian-gantzler]

For components there is a {{component }} helper that you can create components and yield. There is no {{element-modifier }} helper that I am aware of. If one were to become available, they could be yielded. That would be nice as it would remove the need to put api=sortable.api on the item and handle modifiers.

[cah-brian-gantzler]

So ember-modifier says its backward to 3.4, but based on the tests I would says its that addon that is causing the problem.

Given that all the code is new, Another option would be to create another addon called ember-sortable-modifier and move the code there. That might make more sense. Both addons installed at the same time wouldnt be a problem either. The only problem with two separate addons is that any changes would have to be made twice. The current components are basically what modifiers are doing but hard coded to ol and li. Change tag names to use them for tables are not really the encourage route, and if you look at your table you will notice there is actually a div around the tds, thats why its indented and the columns dont line up. The nature of this addon really screams for modifiers and why they were invented.

[ygongdev]

@cah-briangantzler
So I've been thinking about this. First and foremost I really appreciate the effort you have put here.

1. The only downside I see of having both the modifier and non-modifier code in the same addon would inevitable temporarily increase unnecessary bloat b/c people would only use one or the other. Unless tree-shaking can do this?

2. My opinion against having two separate addons is that the new addon will become obsolete b/c eventually this addon will be moved toward Octane when Octane is mature enough (I have personal experience with this as someone who works with large Ember apps that are still far away from accomplishing this). However, I also don't want to punish the other users who are perfectly ready to adopt it.

3. I do not want to delay your work and as a way forward, I was thinking that we can put this work on a separate release branch (v3) that does not interfere with the main release branch (master), so you can start using it. When the time is right, we'll merge the v3 branch back into the main release branch to officially release a v3.

@jgwhite @chriskrycho @lifeart , I would appreciate your thoughts on what the best approach would be here.

[cah-brian-gantzler]

1. Yes tree shaking would knock out the code that isnt used. This point though would seem to favor two addon for exactly this reason.

2. Im suer not how converting the component version to octane would improve the usability. You would still not be able to use it for anything but ol/li elements. Octance would not encourage use of tagName. If anything the modifier version is octane and the original component is the one that would be obsolete.

3. are you saying you people who would want the modifier version would use the branch instead of an npm released version?

Theres a couple things I dont like about the current modifier version and I was able to fix that, look for this PR to be closed and a new one opened. If you are on 3.8 or greater (ie octane) the modifier version is the way to go, there is no reason to use the component version.So two addons are probably better, One if fine but my prediction is that the component version is the one that will fade and the modifier version the one to adopt. Specifically it allows you to use it with tables, the current component version looks like it works with tables, but it actually doesnt correctly.

Look for the new PR in a few, its a much cleaner version.

[chriskrycho]

Given that 3.8 is the current LTS and the modifier works that far back, moving that direction (possibly supporting both during a transition period) is probably the best move. While we shouldn’t intentionally break backwards compatibility, the addon ecosystem also should not (IMO) waste effort supporting unsupported versions of Ember indefinitely. Instead, we should track Ember’s own support. We can cut a 2.x branch and continue to provide big fixes against it for apps like the one we work on, @ygongdev, while supporting the fully Octane-ready version for the ecosystem moving forward (and which we will hopefully be able to use even in our app in less than a month 🤞).

[chriskrycho]

(I’ll have more comments on the design questions around modifiers and components tomorrow!)

[cah-brian-gantzler]

Closing this in favor #345