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

[WIP] [Occlusion Rendering] Preliminary Prototype #44

Open
wants to merge 1 commit into
base: gen-1
Choose a base branch
from

Conversation

kanalveli-ramachandran
Copy link

#42

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

@lolmaus
Copy link
Owner

lolmaus commented Jan 5, 2020

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.

@kjhangiani
Copy link

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.

@kanalveli-ramachandran
Copy link
Author

@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

@lolmaus
Copy link
Owner

lolmaus commented Apr 26, 2020

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.

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

Successfully merging this pull request may close these issues.

3 participants