Page MenuHomePhabricator

Maniphest - fix a bug when selecting all tasks and then dragging them around
ClosedPublic

Authored by btrahan on Nov 11 2013, 11:14 PM.
Tags
None
Referenced Files
F13806063: D7566.diff
Mon, Sep 16, 3:30 AM
F13800860: D7566.diff
Sun, Sep 15, 4:57 AM
Unknown Object (File)
Sat, Sep 14, 10:52 AM
Unknown Object (File)
Thu, Sep 12, 9:43 PM
Unknown Object (File)
Thu, Sep 12, 9:42 PM
Unknown Object (File)
Thu, Sep 12, 9:42 PM
Unknown Object (File)
Thu, Sep 12, 9:42 PM
Unknown Object (File)
Thu, Sep 12, 9:42 PM

Details

Summary

Fixes T4006.

Test Plan

clicked "select all" and dragged around tasks. Noted the task remained selected as I re-ordered, thus keeping hte count accurate. Verified when I hit "batch edit" the right tasks showed up.

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

It's intentional that JX.DOM.hasClass() does not exist, because this hasClass() junk caused a serious and avoidable privacy bug (and some various other nonsense) at Facebook.

Javelin uses sigils instead of CSS classes to rigidly enforce the difference between semantic information and style information in the document. While CSS classes can theoretically handle both, the conflation between semantic and style information in a realistic engineering environment caused a number of problems at Facebook, including a few silly, preventable, and unfortunately severe bugs.

Javelin separates this information into different namespaces, so developers and designers can be confident that changing CSS classes and presentation of a document will never change its semantic meaning. This is also why Javelin does not have a method to test whether a node has a CSS class, and does not have CSS selectors. Unless you cheat, Javelin makes it very difficult to use CSS class names semantically.

http://www.phabricator.com/docs/javelin/article/Concepts_Sigils_and_Metadata.html

Instead, you could write a flag into the node data when the thing is selected:

JX.Stratcom.getData(node).isBatchSelected = true;

...although this will be kind of messy.

We could also merge the maniphest-subpriority-editor and maniphest-batch-selector behaviors; there is little reason for them to be separate now.

Another alternative would be for this behavior to emit an "updated a node" event and have the batch selector listen for it and repaint the correct selection state.

btrahan updated this revision to Unknown Object (????).Nov 11 2013, 11:50 PM

went with the event suggestion. I think the separation of behaviors is still good per the simple behavior thing. I think the updating the node data suggestion was less good than this.