Summary:
Depends on D20475. Ref T13272. Currently, if you `JX.Request` with `data` like `{x: null}`, we submit that as `?x=null`, i.e. as though `null` was the string `"null"`.
This is weird and almost certainly never intended/desiarable. In particular, it causes a bug where panels embedded inside tab panels are incorrectly draggable.
It's possible this breaks something which relied on the buggy behavior, but that seems unlikely.
Test Plan: Tried to drag a panel inside a tab panel, it really truly didn't work.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20476
Summary:
This hasn't been updated in a bit more than a year (last updated in D18594) and we've accumulated a fair number of SQL patches. Update it.
This is mostly automatic (with `bin/storage quickstart`), except:
- Manual edit to one migration for a missed callsite to `DashboardInstall`.
- Replaced two InnoDB tables that still have FULLTEXT indexes with MyISAM (see rP6cedd4a95cfc).
This is not really possible to review and more for reference than examination. `bin/storage quickstart` has historically worked correctly.
Test Plan: I have great faith that `bin/storage quickstart` is a script which creates a big `.sql` file.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D20480
Summary:
Ref T13279. Charting changes alter how the "line-chart" behavior works, but the "Burnup Chart" still relies on the old behavior.
Although I'm intending to remove "Maniphest > Reports" once Facts is a minimally sufficient replacement, copy this behavior to keep it working until we're ready to pull the trigger.
Also fix a leftover typo from D20435.
Test Plan: Viewed a legacy Maniphest burnup rate report.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13279
Differential Revision: https://secure.phabricator.com/D20449
Summary:
Depends on D20439. Ref T13279. Some day, charts will probably need to reload themselves or do a bunch of defer/request-shaping magic when they're on a dashboard with 900 other charts.
Give the controller separate "HTML placeholder" and "actual data" modes, and make the placeholder fetch the data in a separate request.
Then, make the chart redraw if you resize the window instead of staying at whatever size it started as.
Test Plan:
- Loaded a chart, saw it load data asynchronously.
- Resized the window, saw the chart resize.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim
Maniphest Tasks: T13279
Differential Revision: https://secure.phabricator.com/D20440
Summary:
Ref T13279. I think I'm going to fling some stuff at the wall for a bit here and hope most of it sticks, so this series of changes may not be terribly cohesive or focused. Here:
The range of the chart is locked to "[0, 105% of max]". This is trying to make a pleasing extra margin above the maximum value, but currently just breaks charts with negative values. Later:
- I'll probably let users customize this.
- We should likely select 0 as the automatic minimum for charts with no negative values.
- For charts with positive values, it would be nice to automatically pick a pleasantly round number (25, 100, 1000) as a maximum by default.
We don't have any storage for charts yet. Add some. This works like queries, where every possible configuration gets a short URL slug. Nothing writes or reads this yet.
Rename `fn()` to `css_function()`. This builds CSS functions for D3. The JS is likely to get substantial structural rewrites later on, `fn()` was just particularly offensive.
Test Plan: Viewed a fact series with negative values. Ran `bin/storage upgrade`.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13279
Differential Revision: https://secure.phabricator.com/D20438
Summary:
Depends on D20416. Ref T13269. See D20329.
If you try to save an "Assign to" rule with no assignee, we currently replace the control with an "InvalidRule" control that isn't editable. We'd prefer to give you an empty field back and let you pick a different value.
Differentiate between "bad record format" (i.e., we can't really do anything with this) and "bad record value" (i.e., everything is structurally fine, you just typed the wrong thing). In the latter case, we still build a properly typed rule for the UI, we just refuse to update storage until you fix the problem.
Test Plan:
First, hit the original issue and got a nicer UI with a more consistent control width (note full-width error):
{F6374205}
Then, applied the rest of the patch and got a normal "fix the issue" form state instead of a dead-end:
{F6374211}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13269
Differential Revision: https://secure.phabricator.com/D20417
Summary:
Ref T13269. See D20329. When we switch trigger rule control types, reset the rule value.
Also, pick slightly nicer defaults for status/priority.
Test Plan:
- Created a "Change Status To: X" rule.
- Saved it.
- Edited it.
- Selected "Assign to" for the existing action's dropdown.
- Before: tokenizer filled with nonsense.
- After: tokenizer cleared.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13269
Differential Revision: https://secure.phabricator.com/D20416
Summary:
Depends on D20414. Ref T13272. Several minor things here:
- Currently, you can drag panels underneath the invisible "there are no items in this column" div and the "Create Panel / Add Existing Panel" buttons. This is silly; stop it.
- Currently, when viewing a tab panel on a dashboard, you can drag the panels inside it. This is extremely silly. Make "movable" off by default and pass it through the async flow only when we actually need it.
- Make the whole "Add Tab..." virtual tab clickable to open the dropdown. This removes the rare exception/todo combo I added earlier. {key F}
- Add or remove some icons or something.
Test Plan: Moved panels around on dashboards. Tried to drag panels inside tab panels. Added tab. Things were less obviously broken.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20415
Summary:
Depends on D20412. See PHI1147.
- Index the targets of "Add Reviewer", "Add Blocking Reviewer", "Add Auditor", "Add Subscriber", and "Remove Subscriber" Herald rules. My major goal is to get Owners packages. This will also hit projects/users, but we just don't read those edges (for now, at least).
- Add a "Related Herald Rules" panel to Owners Package pages.
- Add a migration to reindex Herald rules for the recent build plan stuff and this, now that such a migration is easy to write.
Test Plan:
Ran migration, verified all rules reindexed.
{F6372695}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D20413
Summary:
Depends on D20411. Ref T13272. Dashboards and panels have new indexes (Ferret and usage edges) that need a rebuild.
For large datasets like commits we have the "activity" flow in T11932, but realistically these rebuilds won't take more than a few minutes on any realistic install so we should be able to just queue them up as migrations.
Let migrations insert a job to basically run `bin/search index --type SomeObjectType`, then do that for dashboards and panels.
(I'll do Herald rules in a followup too, but I want to tweak one indexing thing there.)
Test Plan: Ran the migration, ran `bin/phd debug task`, saw everything get indexed with no manual intervention.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20412
Summary:
Ref T7667. On the road to locking the auth config, also clean up some minor UI issues:
* Only show the warning about not Phacility instance auth if the user isn't a manager (see next diff).
* When rendering more than one warning in the guidance, add bullets.
* I didn't like the text in the `auth.config-lock` config setting.
Test Plan: Loaded the page, saw more reasonable-looking guidance: {F6369405}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T7667
Differential Revision: https://secure.phabricator.com/D20400
Summary:
Depends on D20410. Ref T13272. Dashboards/Panels currently use older "ngram" indexing, which is a less-powerful precursor to Ferret. Throw away the ngram index and provide a Ferret index instead. Also:
- Remove the NUX state, which links to the wrong place now and doesn't seem terribly important.
- Add project tags to the search result list.
- Make the "No Tags" tag a little less conspicious.
Test Plan:
- Indexed dashboards and panels.
- Searched for dashboards and panels via SearchEngine using Ferret "query" field.
- Searched for panels via "Add Existing Panel" datasource typeahead.
- Searched for dashboards via "Add Menu Item > Dashboard" on a ProfileMenu via typeahead.
- Viewed dashboard NUX state (no special state, but no more bad link to "/create/").
- Viewed dashboard list, saw project tags.
- Viewed dashboards with no project tags ("No Tags" is now displayed but less visible).
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20411
Summary: Depends on D20409. Ref T13272. Before "ProfileMenu", dashboards were installed on specific objects using this table. Installs are now handled via ProfileMenu and this table no longer has any meaningful readers. Remove references to the table and destroy it.
Test Plan: Grepped for `DashboardInstall`.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20410
Summary: Depends on D20408. Ref T13272. The actual JS is still a little bit iffy, but this makes the server side "move" operation work correctly by updating it to use the same code as everything else.
Test Plan: Moved panels around on single-column and multi-column dashboards, saw them move to reasonable places and stay there when I reloaded the page.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20409
Summary:
Depends on D20407. Ref T13272. This updates the "add panel" (which has two flavors: "add existing" and "create new") and "remove panel" flows to work with the new duplicate-friendly storage format.
- We now modify panels by "panelKey", not by panel PHID, so one dashboard may have multiple copies of the same panel and we can still figure out what's going on.
- We now work with "contextPHID", not "dashboardID", to make some flows with tab panels (or other nested panels in the future) easier.
The only major remaining flow is the Javascript "move panels around with drag-and-drop" flow.
Test Plan:
- Added panels to a dashboard with "Create New Panel".
- Added panels to a dashboard with "Add Existing Panel".
- Removed panels from a dashboard.
- Added and removed duplicate panels, got a correctly-functioning dashboard that didn't care about duplicates.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20408
Summary:
Ref T13272. See PHI945. Currently, dashboards tend to break when they have duplicate panels. Partly, this is because all the edit operations operate on a "panelPHID", so there's no way to say "remove the copy of panel X at the bottom of the right-hand column", since the operation is `remove(phid)` and that doesn't point at a specific copy of that panel.
In theory, the code is supposed to prevent duplicate panels, but (a) it doesn't always do this successfully and (b) there's no real reason you can't put duplicate panels on a dashboard if you want. There may even be good reason to do this if you have a "random cat picture" panel or something. Even if you aren't doing this on purpose, it's probably better to let you do it and then fix your mistake by removing the panel you don't want than to prevent the operation entirely.
To simplify this whole mess, I want to just support putting the same panel into multiple places on a dashboard. As a first step, change the storage format so each instance of a panel has a unique "panelKey".
Since each instance of each panel now has its own object, this will also let us give particular instances of panels things like "automatic refresh time" (T5514) or "custom name for this panel on this dashboard" later, if we want. Not clear these are valuable but having this capability can't hurt.
Test Plan:
- `var_dump()`'d the migration, looked at all the results.
- Ran the migration.
NOTE: This breaks dashboards on its own since none of the other code has been changed yet, see followups.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20405
Summary:
Depends on D20398. Ref T13272. Fixes T6018. Previously, panels showed "used on dashboards: x, y", but this did not include cases where a panel was used by another container panel (today, a tab panel).
Do edge indexing when a dashboard or panel is saved, then pull the edges on the Panel page so we can provide a full list of uses.
Test Plan: {F6369289}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272, T6018
Differential Revision: https://secure.phabricator.com/D20399
Summary:
Depends on D20397. Ref T13272. Similar to the recent "where are Herald rules used" stuff, show which menus Dashboards are installed in.
This is mostly straightforward, except that I pulled some of the Herald logic into a parent class so it could be shared.
Test Plan: {F6369164}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20398
Summary:
Depends on D20396. Ref T13272. Currently, using the dropdowns to edit a tab panel from a dashboard redirects you to the tab panel page.
Instead, redirect back to the context page (usually, a dashboard -- but theoretically a containing tab panel, since you can put tab panels inside tab panels).
Also, fix some JS issues with non-integer panel keys. I've moved panel keys from "0, 1, 2, ..." to having arbitrary keys to make some operations less flimsy/error-prone, but this needs some JS tweaks.
Test Plan: Edited a tab panel from a dashboard, got sent sensibly back to the dashboard.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20397
Summary:
Ref T13272. In edit mode, tab panels now have a dropdown menu. However, this sort of overrlaps with the actual action of clicking the tab to select it.
Separate these into different click targets so that "select tab X" and "open dropdown menu for X" are different operations.
This is more work than it appears because:
- We have an "action icon" already, used when you put a dashboard on a portal/home to create an "Edit" link. It makes sense to attach dropdowns to this, but it has some hard-coded stuff.
- In applications with a "Create <thing>" in the crumbs (like Maniphest), we may use a dropdown menu if there are multiple create forms available. However, this menu renders in a weird way by reading all the properties out of an actual "View" object and building something else.
- The "list of tabs" stuff shares code with different "list of tabs" navigation used by Diffusion and Instances.
..but I think I fixed everything and didn't break anything.
Test Plan:
- Clicked "select tab" and "open dropdown menu" as separate actions.
- Viewed Diffusion, Maniphest with multiple create forms, Instances.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20396
Summary:
Ref T13275. Add portals to the search index so that:
- they show up in fulltext global search; and
- the typeahead actually uses an index.
Also make them taggable with projects as an organizational aid.
Test Plan: Indexed portals with `bin/serach index`, searched for a portal with "Query", with fulltext search in main menu, with typehead on "Install Dashboard...", changed the name of a portal and searched again to check that the index updates properly.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13275
Differential Revision: https://secure.phabricator.com/D20389
Summary:
Ref T13263.
- Make the user profile section of the "Profile" dropdown menu have a transparent background, not a white background. This is a pre-existing issue. This is normally hard to see, but visible on Workboards with custom background colors.
- Fix an alignment issue with the little "V" caret in the search scope dropdown. This is a recent issue caused by some tab-caret CSS I added recently for tabbed dashboard panels.
Test Plan:
Before:
{F6367723}
After:
{F6367724}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13263
Differential Revision: https://secure.phabricator.com/D20388
Summary:
Depends on D20383. Ref T13272. Fixes T12363. See PHI997. This gets the edit flows for tab panels functional again. They aren't //nice//, and a lot of the workflows are fairly janky: for example, most of them end up with you on the tab panel's page, which isn't useful if you started on a dashboard page.
However, these flows were extremely janky before anyway (see T12363) and I suspect this is a net improvement even though it's a bit of a mess. I anticipate cleaning this up bit-by-bit in future diffs.
Test Plan: {F6366372}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272, T12363
Differential Revision: https://secure.phabricator.com/D20384
Summary:
Depends on D20382. Ref T13272. When something near the edge of the screen has a dropdown menu, we currently may render the menu offscreen.
Instead, keep the menu onscreen.
(This is happening because I'm adding dropdown menus to tab query panels.)
Test Plan:
Before:
{F6363339}
After:
{F6363340}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20383
Summary:
Depends on D20372. Ref T13272.
- There's a very heavy dropshadow on panels right now that looks out of place. Reduce it a bit.
- Panels currently have unlabeled pencil and trash icons. Turn this into a menu. I'm likely planning to add options like "Change Query..." to this menu to make managing some types of panels easier.
Test Plan: {F6332838}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20373
Summary:
Depends on D20362. Ref T13272. Currently, Dashboards have an "Install Dashboard" flow which is pretty janky and only allows you to install things to the home page.
Instead, allow users to install things to any valid target (home, favorites, portals, projects). This also provides URIs like `dashboard/install/1/home/personal/` which allow you to link users to an "install a dashboard" page; this may or may not get used.
Test Plan: Installed dashboards on home, favorites, projects, and portals.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20364
Summary:
See <https://discourse.phabricator-community.org/t/non-functional-actions-menu-on-live-phame-views/2593>. Several layers here:
The "Actions" button is broken because a menu behavior is failing, since we aren't rendering the menu.
When a behavior fails to initialize, catch and log the exception and continue. Previously, we stopped initializing behaviors if any failed, but behaviors are usually independent and continuing with an explicit exception seems reasonable.
Give "JX.log()" some "sprintf()" semantics to make logging the behavior failure easier. We can probably afford these extra 200 bytes now in 2019.
This fixes the button and gives us explicit errors in the log. So far, so good.
Then, when a page won't render chrome, don't try to render the main menu. This fixes the actual errors (we no longer try to initialize menu behaviors for nodes which don't exist).
Completely hide the "Actions" and "Comment" flows if the viewer isn't logged in. Although this isn't completely consistent with other applications, I think it's more appropriate for Phame. In applications like Maniphest, we show a full set of controls (but disable them) so that users who are not currently logged in have a clear path to interact with the content, under the assumption that this is a relatively common workflow. This is probably less common for Phame, where we expect most anonymous viewers not to log in or interact.
Finally, parametrize a one-off border color and add a border under the crumbs at the top of the page.
Test Plan:
- Viewed a "Live" Phame blog post page, clicked "Actions", got a dropdown.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20378
Summary:
Depends on D20353. Ref T13275. This is just some small quality-of-life fixes:
- When you add items to menus, they currently go below the "Edit Menu/Manage Menu" links by default. This isn't a very good place for them. Instead, lock "edit" items to the bottom of the menu.
- Lock profile pictures to the top of the menu. This just simplifies things a little.
- Show more iconography hints on the "edit menu items" UI.
- Add a "drag stuff to do things" hint if some stuff can be dragged.
Test Plan:
- Added new items to a Portal, they didn't go to the very bottom. Instead, they went above the "Edit/Manage" links; a sensible place for them.
- Viewed the "edit menu items" screen, saw more hints and visual richness.
- Viewed/edited Home, Projects, Portals, Favorites
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13275
Differential Revision: https://secure.phabricator.com/D20355
Summary:
Ref T13275. Today, you can build a custom page on the home page, on project pages, and in your favorites menu.
PHI374 would approximately like to build a completely standalone custom page, and this generally seems like a reasonable capability which we should support, and which should be easy to support if the "custom menu" stuff is built right.
In the near future, I'm planning to shore up some of the outstanding issues with profile menus and then build charts (which will have a big dashboard/panel component), so adding Portals now should let me double up on a lot of the testing and maybe make some of it a bit easier.
Test Plan:
Viewed the list of portals, created a new portal. Everything is currently a pure skeleton with no unique behavior.
Here's a glorious portal page:
{F6321846}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13275
Differential Revision: https://secure.phabricator.com/D20348
Summary:
See PHI1185, which reports a performance issue with "(" in remarkup in certain contexts.
I can't reproduce the performance issue, but I can reproduce the autosuggester incorrectly remaining active and swallowing return characters.
When the user types `(` or `[`, we wait for a prefix for the `((` (Phurl) or `[[` (Phriction) rules. We currently continue looking for that prefix until a character is entered that explicitly interrupts the search.
For example, typing `(xxx<return>` does not insert a return character, because we're stuck on matching the prefix.
Instead, as soon as the user has entered text that we know won't ever match the prefix, deactivate the autocomplete. We can slightly cheat through this by just looking for at least one character of text, since all prefixes are exactly one character long. If we eventually have some kind of `~~@(xyz)` rule we might need to add a more complicated piece of rejection logic.
Test Plan: Typed `(xxx<return>`, got a return. Used `((` and `[[` autosuggest rules normally. Used `JX.log()` to sanity check that nothing too crazy seems to be happening.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20365
Summary: Depends on D20345. Use a narrower layout for very large graphs to save some space.
Test Plan:
Before:
{F6320215}
After:
{F6320216}
This does not affect smaller graphs.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20346
Summary:
See downstream <https://phabricator.wikimedia.org/T171648>. The `T123 Task Name` column in graphs can currently fold down to 0 pixels wide.
Although it's visually nice to render this element without a scroll bar when we don't really need one, the current behavior is excessive and not very useful.
Instead, tweak the CSS so:
- This cell is always at least 320px wide.
- After 320px, we'll overflow/ellipsis the cell on small screens.
This generally gives us better behavior:
- Small screens get a scrollbar to see a reasonable amount of content.
- The UI doesn't turn into a total mess if one task has a whole novel of text.
Test Plan:
Old behavior, note that there's no scrollbar and the cell is so narrow it is useless:
{F6320208}
New behavior, same default view, has a scrollbar:
{F6320209}
Scrolling over gives you this:
{F6320210}
On a wider screen (this wide or better), we don't need to scroll:
{F6320211}
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20345
Summary:
Fixes T13273. This element is a bit weird, but I think I fixed it without breaking anything.
The CSS is used by project hovercards and user hovercards, but they each have a class which builds mostly-shared-but-not-really-identical CSS, instead of having a single `View` class with modes. So I'm not 100% sure I didn't break something obscure, but I couldn't find anything this breaks.
The major issue is that all the text content has "position: absolute". Instead, make the image "absolute" and the text actual positioned content. Then fix all the margins/padding/spacing/layout and add overflow. Seems to work?
Plus: hide availability for disabled users, for consistency with D20342.
Test Plan:
Before:
{F6320155}
After:
{F6320156}
I think this is pixel-exact except for the overflow behavior.
Also:
- Viewed some other user hovercards, including a disabled user. They all looked unchanged.
- Viewed some project hovercards. They all looked good, too.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13273
Differential Revision: https://secure.phabricator.com/D20344
Summary:
See downstream <https://phabricator.wikimedia.org/T201480>. Searching for things on mobile is a significant challenge because clicking the "Magnifying Glass" icon shows and then immediately hides the menu. I believe some aspect of iOS event handling has changed since this was originally written.
At some point, I'd like to rewrite this to work more cleanly and get rid of `jx-toggle-class`. In particular, it isn't smart enough to know that it should be modal with other menus, so you can get states like this by clicking multiple things:
{F6320110}
This would also probably just look and work better if it was an inline element that showed up under the header instead of a floating dropdown element.
However, I'm having a hard time getting the Safari debugger to actually connect to the iOS simulator, so take a small step toward this bright future and fix the immediate problem for now: toggle on click instead of mousedown/touchstart.
This means the menu opens ~100ms later, but actually works. Big improvement!
I'd like to move away from "jx-toggle-class" anyway (it usually isn't sophisticated enough to fully describe a behavior) so reducing complexity here seems good. It isn't used in //too// many places so this is unlikely to have any negative effects, I hope.
Test Plan: On iOS simulator, clicked the magnifying glass icon in the main menu to get a search input. Before: got a search input for a microsecond. After: actually got a search input.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20343
Summary: Ref T13269. Make it visually more clear that the "Trigger" and "New Task / Edit / Bulk" dropdown menu items are buttons, not status icons or indicators of some kind.
Test Plan: {F6313872}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13269
Differential Revision: https://secure.phabricator.com/D20332
Summary: Ref T13269. I refactored this late in the game to organize things better and add table cells around stuff, and accidentally broke the relationship between the "Rule Type" selector and the value selector.
Test Plan: Switched rule type selector from "Change Status" to "Play Sound", saw secondary control update properly.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13269
Differential Revision: https://secure.phabricator.com/D20326
Summary:
Depends on D20321. Fixes T12175. Ref T13074. Now that before/after PHIDs are suggestions, we can give the server a more complete view of what the client is trying to do so we're more likely to get a good outcome if the client view is out of date.
Instead of passing only the one directly adjacent card PHID, pass all the card PHIDs that the client thinks are in the same group.
(For gigantic columns with tens of thousands of tasks this might need some tweaking -- like, slice both lists down to 10 items -- but we can cross that bridge when we come to it.)
Test Plan:
- Dragged some cards around to top/bottom/middle positions, saw good positioning in all cases.
- In two windows, dragged stuff around on the same board. At least at first glance, conflicting simultaneous edits seemed to do reasonable things.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13074, T12175
Differential Revision: https://secure.phabricator.com/D20322
Summary:
Depends on D20320. Ref T12175. Ref T13074. Currently, when you move a card between columns, the internal transaction takes exactly one `afterPHID` or `beforePHID` and moves the card before or after the specified card.
This is a fairly strict interpretation and causes a number of practical issues, mostly because the user/client view of the board may be out of date and the card they're dragging before or after may no longer exist: another user might have moved or hidden it between the last client update and the current time.
In T13074, we also run into a more subtle issue where a card that incorrectly appears in multiple columns fatals when dropped before or after itself.
In all cases, a better behavior is just to complete the move and accept that the position may not end up exactly like the user specified. We could prompt the user instead:
> You tried to drop this card after card X, but that card has moved since you last loaded the board. Reload the board and try again.
...but this is pretty hostile and probably rarely/never what the user wants.
Instead, accept a list of before/after PHIDs and just try them until we find one that works, or accept a default position if none work. In essentially all cases, this means that the move "just works" like users expect it to instead of fataling in a confusing/disruptive/undesirable (but "technically correct") way.
(A followup will make the client JS send more beforePHIDs/afterPHIDs so this works more often.)
We could eventually add a "strict" mode in the API or something if there's some bot/API use case for precise behavior here, but I suspect none exist today or are (ever?) likely to exist in the future.
Test Plan:
- (T13074) Inserted two conflicting rows to put a card on two columns on the same board. Dropped one version of it underneath the other version. Before: confusing fatal. After: cards merge sensibly into one consistent card.
- (T12175) Opened two views of a board. Moved card A to a different column on the first view. On the second view, dropped card B under card A (still showing in the old column). Before: confusing fatal. After: card ended up in the right column in approximately the right place, very reasonably.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13074, T12175
Differential Revision: https://secure.phabricator.com/D20321
Summary:
Depends on D20308. Ref T5474. The element which previews what will happen when you drop a task somewhere can cover the bottom part of the rightmost column on a workboard.
To fix this, I'm trying to just fade it out if you put your cursor over it. I tried to do this in a simple way previously (":hover" + "opacity: 0.25") but it doesn't actually work because "pointer-events: none" stops ":hover" from working.
Instead, do this in Javascript. This is a little more complicated but: it works; and we can do the fade when you get //near// the element instead of actually over it, which feels a little better.
Test Plan:
- Shrank window to fairly small size so that the preview could cover up stuff on the workboard.
- Dragged a card toward the rightmost column.
- Before: drop action preview covered some workboard stuff.
- After: preview faded out as my cursor approached.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T5474
Differential Revision: https://secure.phabricator.com/D20320
Summary:
In some cases, we show a limited number of one type of object somewhere else, like "Recent Such-And-Such" or "Herald Rules Which Use This" or whatever.
We don't do a very good job of communicating that these are partial lists, or how to see all the results. Usually there's a button in the upper right, which is fine, but this could be better.
Add an explicit "more stuff" button that shows up where a pager would appear and makes it clear that (a) the list is partial; and (b) you can click the button to see everything.
Test Plan: {F6302793}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D20315
Summary:
Ref T5474. In 99% of cases, a separate "archived/active" status for triggers probably doesn't make much sense: there's not much reason to ever disable/archive a trigger explcitly, and the archival rule is really just "is this trigger used by anything?".
(The one reason I can think of to disable a trigger manually is because you want to put something in a column and skip trigger rules, but you can already do this from the task detail page anyway, and disabling the trigger globally is a bad way to accomplish this if it's in use by other columns.)
Instead of adding a separate "status", just track how many columns a trigger is used by and consider it "inactive" if it is not used by any active columns.
Test Plan: This is slightly hard to test exhaustively since you can't share a trigger across multiple columns right now, but: rebuild indexes, poked around the trigger list and trigger details, added/removed triggers.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T5474
Differential Revision: https://secure.phabricator.com/D20308
Summary:
Ref T5474. Allow columns to play a sound when tasks are dropped.
This is a little tricky because Safari has changed somewhat recently to require some gymnastics to play sounds when the user didn't explicitly click something. Preloading the sound on the first mouse interaction, then playing and immediately pausing it seems to work, though.
Test Plan: Added a trigger with 5 sounds. In Safari, Chrome, and Firefox, dropped a card into the column. In all browsers, heard a nice sequence of 5 sounds played one after the other.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T5474
Differential Revision: https://secure.phabricator.com/D20306
Summary:
Ref T13074. If you hit a prompt on a drop operation (today: MFA; in the future, maybe "add a comment" or "assign this task"), we currently leave the board in a bad semi-frozen state if you cancel the workflow by pressing "Cancel" on the dialog.
Instead, put things back the way they were.
Test Plan: Dragged an MFA-required card, cancelled the MFA prompt, got a functional board instead of a semi-frozen board I needed to reload.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13074
Differential Revision: https://secure.phabricator.com/D20305
Summary:
Ref T5474. The first rough cut of triggers showed some of the trigger rules in a tooltip when you hover over the "add/remove" trigger menu.
This isn't great since we don't have much room and it's a bit finnicky / hard to read.
Since we have a better way to show effects now in the drop preview, just use that instead. When you hover over the trigger menu, preview the trigger in the "drop effect" element, with a "Trigger: such-and-such" header.
Test Plan:
- This is pretty tough to screenshot.
- Hovered over menu, got a sensible preview of the trigger effects.
- Dragged a card over the menu, no preview.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T5474
Differential Revision: https://secure.phabricator.com/D20304
Summary:
Ref T5474. This provides a Herald-like UI for editing workboard trigger rules.
This probably has some missing pieces and doesn't actually save anything to the database yet, but the basics at least roughly work.
Test Plan: {F6299886}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T5474
Differential Revision: https://secure.phabricator.com/D20301
Summary:
Ref T10335. When you (for example) drag a "Resolved" task into a column with "Trigger: change status to resolved.", don't show a hint that the action will "Change status to resolved." since this isn't helpful and is somewhat confusing.
For now, the only visibility operator is "!=" since all current actions are simple field comparisons, but some actions in the future (like "add subscriber" or "remove project") might need other conditions.
Test Plan:
Dragged cards in ways that previously provided useless hints: move from column A to column B on a "Group by Priority" board; drag a resolved task to a "Trigger: change status to as resolved" column. Saw a more accurate preview in both cases.
Drags which actually cause effects still show the effects correctly.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10335
Differential Revision: https://secure.phabricator.com/D20300
Summary:
Ref T10335. Ref T5474. When you drag-and-drop a card on a workboard, show a UI hint which lists all the things that the operation will do.
This shows: column moves; changes because of dragging a card to a different header; and changes which will be caused by triggers.
Not implemented here:
- Actions are currently shown even if they have no effect. For example, if you drag a "Normal" task to a different column, it says "Change priority to Normal.". I plan to hide actions which have no effect, but figuring this out is a little bit tricky.
- I'd like to make "trigger effects" vs "non-trigger effects" a little more clear in the future, probably.
Test Plan:
Dragged stuff between columns and headers, and into columns with triggers. Got appropriate preview text hints previewing what the action would do in the UI.
(This is tricky to take a screenshot of since it only shows up while the mouse cursor is down.)
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10335, T5474
Differential Revision: https://secure.phabricator.com/D20299
Summary:
Depends on D20278. Ref T5474. This change creates some new empty objects that do nothing, and some new views for looking at those objects. There's no actual useful behavior yet.
The "Edit" controller is custom instead of being driven by "EditEngine" because I expect it to be a Herald-style "add new rules" UI, and EditEngine isn't a clean match for those today (although maybe I'll try to move it over).
The general idea here is:
- Triggers are "real" objects with a real PHID.
- Each trigger has a name and a collection of rules, like "Change status to: X" or "Play sound: Y".
- Each column may be bound to a trigger.
- Multiple columns may share the same trigger.
- Later UI refinements will make the cases around "copy trigger" vs "reference the same trigger" vs "create a new ad-hoc trigger" more clear.
- Triggers have their own edit policy.
- Triggers are always world-visible, like Herald rules.
Test Plan: Poked around, created some empty trigger objects, and nothing exploded. This doesn't actually do anything useful yet since triggers can't have any rule behavior and columns can't actually be bound to triggers.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T5474
Differential Revision: https://secure.phabricator.com/D20279
Summary: See downstream <https://phabricator.wikimedia.org/T166358>. The notifications menu is missing some CSS to color and style values in stories like "renamed task from X to Y".
Test Plan:
Before:
{F6302123}
After:
{F6302122}
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20310
