ember-drag-sort icon indicating copy to clipboard operation
ember-drag-sort copied to clipboard

[WIP] [Occlusion Rendering] Preliminary Prototype

Open kanalveli-ramachandran opened this issue 4 years ago • 4 comments

#42

This is a prototype implementation using https://github.com/html-next/vertical-collection

kanalveli-ramachandran avatar Dec 13 '19 07:12 kanalveli-ramachandran

This is very impressive!

I have some concerns:

1. Excessive HTML Markup

Here's how a normal drag-sort list looks like:

<div id="ember5" class="dragSortList -draggingEnabled -vertical ember-view">
  <div draggable="true" id="ember7" class="dragSortItem -isTargetNOT 0 ember-view">
    <div class="the-item">Foo</div>
  </div>
  
  <div draggable="true" id="ember9" class="dragSortItem -isTargetNOT 1 ember-view">
    <div class="the-item">Bar</div>
  </div>
  
  <div draggable="true" id="ember11" class="dragSortItem -isTargetNOT 2 ember-view">
    <div class="the-item">Baz</div>
  </div>
  
  <div draggable="true" id="ember13" class="dragSortItem -isTargetNOT 3 ember-view">
    <div class="the-item">Quux</div>
  </div>
</div>

Here's how the same list looks in your branch:

<div id="ember150" class="dragSortList -draggingEnabled -vertical ember-view">
  <occluded-content class="occluded-content" style="height: 0px;"></occluded-content>
  
  <div>
    <div draggable="true" id="ember154" class="dragSortItem -isTargetNOT 0 ember-view">
      <div class="the-item">Foo</div>
    </div>
  </div>
  
  <div>
    <div draggable="true" id="ember155" class="dragSortItem -isTargetNOT 1 ember-view">
      <div class="the-item">Bar</div>
    </div>
  </div>
  
  <div>
    <div draggable="true" id="ember156" class="dragSortItem -isTargetNOT 2 ember-view">
      <div class="the-item">Baz</div>
    </div>
  </div>
  
  <div>
    <div draggable="true" id="ember157" class="dragSortItem -isTargetNOT 3 ember-view">
      <div class="the-item">Quux</div>
    </div>
  </div>
  
  <occluded-content class="occluded-content" style="height: 0px;"></occluded-content>
  <!---->
</div>

That's with virtual rendering disabled.

Given that virtual rendering is a relatively rare case, I'd rather implement it with a separate component, e. g. drag-sort-list-virtual.

2. Angle brackets

vertical-collection uses positional arguments, which would prevent us from converting to angle bracket components.

This is not a big deal, given that conversion to Glimmer components will be difficult and unlikely to happen soon.

On the other hand, the addon does not appear to be well-maintained: no commits in half a year, lots of pending PRs, failing build...

3. vertical-collection options

I believe, all vertical-collection options should be exposed.

lolmaus avatar Jan 05 '20 10:01 lolmaus

We are also running into the same performance bottlenecks with large lists (not exclusively with ember-drag-sort, but we do experience it with this library as well).

I agree that it should be opt-in or a separate component, but the library is actually very solid despite the lack of activity. We use it throughout our app and it has made it possible to render lists of even 20-30k with neglible performance penalties. Any moderately complex component in drag-sort-list already has rendering lag even in lists as small as ~30-50 so this could be a huge win.

Also note that while vertical-collection has not had activity in some time, it is quite stable when it works and the core developers are part of / close to the ember core team (runspired, etc)

I'm about to attempt a variation of this PR in our app by extending the components and/or forking - will report back with my findings soon.

kjhangiani avatar Apr 24 '20 21:04 kjhangiani

@kjhangiani +1 about the stability of vertical-collection

@lolmaus

This PR was just a jab at the possibilities.

One of my colleagues has worked on incorporating your suggestions with the following changes in our fork,

  1. An extended component 'drag-sort-occluded'
  2. Expose all options accepted by vertical-collection

Lets discuss further once he raises the sync request from the fork

kanalveli-ramachandran avatar Apr 25 '20 10:04 kanalveli-ramachandran

I still have concerns about vertical-collection. It may be stable, but it's not getting updated for Octane.

This addon still heavily relies on Ember components and would need a major refactoring to migrate to Glimmer components. So I'm kinda not in the position to complain. :)

But on the other hand I wouldn't want the addon to be falling behind in two things at once instead of two.

So my question to you guys is: can we make a dependency on vertical-collection optional? Will the ember-drag-sort addon keep working as usual when vertical-collection is not part of the build? I think that might be possible if we use in in a separate list component.

In that case it would be much easier for me to accept it into the codebase.

lolmaus avatar Apr 26 '20 06:04 lolmaus