Skip to content

Tracking: sortable API #69

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

Closed
cormacrelf opened this issue Aug 18, 2018 · 4 comments
Closed

Tracking: sortable API #69

cormacrelf opened this issue Aug 18, 2018 · 4 comments

Comments

@cormacrelf
Copy link
Owner

cormacrelf commented Aug 18, 2018

This is a tracking issue for the sortable functionality.

It won’t be part of the core package, it will be separate. At this stage I’m looking for feedback and comments on API design. Please 👍🏻 if you want to beta test the basic directive/component stuff.

What is it at the moment?

The best intro to the WIP functionality is reading the Kanban example source code. A quick rundown:

  • It's currently called angular-skyhook-card-list, but this name is bad
  • CardListDirective
    • Attach to DOM container element
    • Expects a list ID, a SortableSpec, and a DND type (like drag source / drop target types)
    • Expects you to render the list emitted by SortableSpec.getList inside it, each item having [cardRenderer]="cardList.contextFor(data, index)"
    • Expects you to attach the cardRenderer's source to the DOM
    • Handles empty list case
  • CardListComponent
    • Extends the directive, but makes it a little easier by handling the ngFor and the cardRenderer context.
    • Accepts cardTemplate as a content child to facilitate this
  • SortableSpec
    • DIY list reordering. You control everything, so you can use Immutable or NgRx or whatever.
    • kinda like DragSource + DropTarget, but gives you monitor.getItem directly, and only calls hover when you need to render the placeholder somewhere else.
  • New: NgRxSortable + SortableAction (see below)

SortableSpec

The main factor to consider in decision-making is that implementing SortableSpec according to the needs of the library as it exists now is difficult. The library is fundamentally more flexible than the alternatives which all do at least some array work themselves (ex. Ng2-Dragula, 100% of it; Cdk-experimental, apparently the hover states are handled).

It probably isn't possible to handle all the hover states internally / simplify the spec to just confirmed moves. How would you do that while supporting arbitrary ngFor/deep children/some elements fixed? The relevant CDK code excludes some of these possibilities in its internal implementation — it just runs insertBefore with some prerendered Dom.

SortableSpec requires, currently:

  1. trackBy should give unique outputs / be injective like a good trackByFn
  2. Must produce a list with dragged element still present in beginDrag, or not output any change at all
  3. Must handle multiple listIds, meaning there’s a minimum backing store complexity of { [k: string]: Data[] } or at least a list of lists (like the Kanban example)
  4. Must be able to revert on endDrag
  5. Must hover or drop in location described by hovering item, without losing 2
  6. ANY modifications must be presented through an Observable (probably a BehaviorSubject or similar) producing a new value
  7. Optional: Correctly handle items dragged from external sources.

2 means simple array mutations are out, and slice(0) is in. 6 is a hurdle because few people in the wild are comfortable enough with Rx to do this on their own. Nevertheless, every item can be tricky if you attack the problem blindly.

Copy/clone support

Should SortableSpec include copy/clone functionality? Currently those features appear to require internal support from the directives due to Chrome bugs. Needs a second look.

If you add copy/clone, there are actually three more requirements for SortableSpec implementations:

  1. copy returns boolean, mainly a burden if you’re trying to be generic
  2. clone(x) -> x’ returns x’ !== x, trackBy(x’)!==trackBy(x)
  3. Hard part: Check if item.isCopy during beginDrag/hover/drop, and completely change behaviour. May be coalesced with external drags functionality, since it’s kinda similar. JIT strategy below makes this easier.

SortableSpec implementation strategies

There are two main strategies I’ve used so far.

Direct (not very good)

  1. keep a canonical copy, call it beforeDrag
  2. produce a new list each time by immutably moving the card from the beforeDrag structure to a new one on each hover. hover() { current = move(beforeDrag, ...) }
  3. drop() { beforeDrag = move(beforeDrag, ...); }

Pros: no NgRx needed.
Cons: much more complicated to write, and difficult to get bug-free, especially when supporting externals and clones. Half of the moveCard function is in an if block.

Just-In-Time

Similar to react-sortable-tree. See Kanban again.

  1. keep a canonical data store intact
  2. Clone store + remove item in beginDrag, but keep a reference to the item
  3. Re-add it on-demand ('JIT'), in a selector.

Pros: Simple hover implementation that just updates the item to be inserted later. Simple handling of externals and clones; just don’t delete the original and proceed as usual.
Cons: kinda need NgRx and all that baggage.

(Side note: could you do step 3 of JIT without requiring users to implement it themselves? Probably not. We want to support Immutable.js, for example, which doesn't have the same insert API as arrays.)

Shipping SortableSpec implementations?

As far as I can see, there are no satisfactory reasons for actually shipping any complex SortableSpec implementations. Any such implementation would need to be complete, dependency-free (manual immutables, no NgRx), and configurable (how? The directives I wrote to do this are really bad, and there’s no easy way to make a spec that updates a component variable without weird callbacks or subscriptions.). That would be a maintenance nightmare, and because the code would look really scary (see: the other requirements), users would make feature requests instead of building features themselves.

I also really don’t want people using whatever spec that’s available as an alternate data store. Sortable should work with your existing data. The Kanban example store is great because it takes existing data and builds a spec that simply interacts with that, and you have complete control.

On the other hand, people don't typically want to use more than one dnd library, as it has a cost on bundle size. There is a mid-point between extensibility and ease of use that might not be covered without any shipping specs that are easy to use.

SharedSortableSpec -- potential

The most promising spec to ship would be the ng2-dragula replacement, SharedSortableSpec. It would probably be rewritten to resemble a mini-NgRx with the JIT strategy, and the directive that applies it would be subsumed within CardListDirective so it wouldn't be nearly as hacky.

Pros: worthy replacement for ng2-dragula and approximately as simple as all the other solutions out there.
Cons: maintenance, non-extensibility, and feature creep. Basically, all the reasons why you wouldn't use other libraries. Because it's easier than using ngrx, people would choose it as the default, and wouldn't be able to add features.

NgRX helpers (edit: DONE)

I have one last idea for what to ship:

  • An NgRx Action class, including the following
    • event type (beginDrag, hover, etc)
    • item type
    • DraggedItem object
    • (instead of the silly one class per event-itemtype!)
  • A SortableSpec with customisable trackBy that takes a Store, and dispatches that action when the spec is called

This would remove about 50 lines of boilerplate from the kanban demo. More importantly, it would represent a focus on advanced NgRx-based use cases and not simple one-component lists, for which there are abundant easier alternatives.

@ghost ghost added the triage label Aug 18, 2018
@ghost ghost removed the triage label Aug 18, 2018
cormacrelf added a commit that referenced this issue Aug 19, 2018
…rs much easier

(~50 LOC of boilerplate saved in the Kanban example, as predicted in #69)
@cormacrelf
Copy link
Owner Author

cormacrelf commented Aug 21, 2018

Choices left to make

  • should library be horizontal-CSS-agnostic? Edit: yes. Currently forces flexbox.
  • Any way to simplify the context passing other than using the component? DI? Edit: no. It would mean passing two attrs (data, id), instead of one, so not a huge win.
  • how do you make placeholders disappear when you mouse-out of ALL lists ('spill')? I think this is userspace, by storing 'spill state' and using t=dropTarget(..., {hover: ... if isOver shallow, set spill=true}); t.connectDropTarget(document.body). Edit: yep. Made a helper in library, but it does basically this. spillTarget(this.dnd, "MYCARD", { hover: item => ... })
  • Copy/clone support built-in? Or userspace? Edit: userspace. 64c1421
  • Simple usage without observables? Maybe make getList optional? Edit: 4049d61
  • Should it include a basic “just reorder my array please” implementation to replace ng2-dragula and its ilk? I built one, but I don't want to maintain it or version it alongside skyhook core. Edit: NO. No need, now that you don't need observables. aac909d - plus simple.component
  • trackBy: leave as it is (item)=>any, or follow the extremely dumb but compatible Angular way (index, item)=>any? Edit: leave it.
  • should DraggedItem.size be a real DOMRect? Would be better for using animation? Edit: no. The DOMRect would have to be kept up to date. No.
  • Make isEmpty configurable? Edit: not in 1.0.
  • Is integrating data from a Store into an NgRxSortable too awkward? Yeah it is a little. But just isolate it to a *Service class.
  • Is horizontal better set on cardRenderer? Does it need to be consistent? What about the immediate shift on hover? It would make it easier to add 'grid mode' later. No. If it were customizable in future, you would only ever need to customize based on indexes, which you would be given.
  • Should the library include helpers for integrating external drag sources? What does that look like? A DraggedItem constructor, or a wrapper for SkyhookDndService.dragSource like the drilldown / hover example? Most of the problem is that simply having { beginDrag: () => wrapper(...), } wouldn't call SortableSpec.endDrag.
  • wrappers - how do they look in TypeScript? No generic enough formulation.
  • trello's horiz board>lists, and many other fixed size lists like Things for macOS (oneline with ...), trigger a reorder as soon as you hover on a neighbour. Works for fixed-onaxis-dimension only. Support that? Yes
  • NgRx helpers in a sub package? (import ... from '@angular-skyhook/sortable/ngrx’). No

Todos

  • SortableSpec and the basic directive and component
  • NgRx helpers to make building reducers easier: cc26986
  • remove flexbox in horizontal mode, allow user to specify display: grid, floats, etc
  • fix list padding in kanban's <skyhook-preview>
  • try out spill state / document approach -> now includes spillTarget helper
  • explore userspace copy/clone
  • Chrome dragend bug
  • Make getList optional
  • Move dnd type into spec
  • Rename everything. Incl DraggedItem -> SortableItem so beta can start
  • fixed-size hover support
  • Need to test external items working with kanban.
  • Re-implement isEmpty to use Symbol.iterator
  • Rename 'render' stuff, because it does not render anything. ssChild?

@cormacrelf cormacrelf changed the title Tracking/discussion: sortable API Tracking: sortable API Aug 23, 2018
@cormacrelf
Copy link
Owner Author

cormacrelf commented Sep 11, 2018

@Digister beta is out. Moved to scoped packages too.

https://cormacrelf.github.io/angular-skyhook/sortable/

@jorroll
Copy link
Contributor

jorroll commented Sep 12, 2018

So I didn't think I was interested in this, because, at the moment, I'm pretty happy with the simple sortable directive I made on top of angular-skyhook, but your use cases blurb in the docs is compelling. I'm refactoring my apps backend at the moment, but I'll check this out in a few weeks.

Thanks! 👍

@cormacrelf
Copy link
Owner Author

@thefliik Glad to hear it! When you do, let me know if there are any ways you think it could improve!

(You should also know that this is just one big 'basic sortable' under the hood, but easier, more powerful and requiring less plumbing. Ideally refactoring to use it should involve deleting a bunch of code, mainly that huge 'hover' function.)

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

2 participants