1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-05 05:02:44 +01:00
Commit graph

55 commits

Author SHA1 Message Date
epriestley
8bdc713352 Make the "Keyboard Shortcuts" dialog in Differential less hideous
Summary:
Ref T13515. Adding "\" ("Open in External Editor") made this slighlty worse, but it was already pretty bad.

Long ago the keys had a special style on them, but this got changed and dropped somewhere around D16568 -- although at the time, I think they still had a grey background (see T11654).

Some later change removed this background.

Put the background back and separate the keystrokes into groups.

Test Plan: {F7370615}

Maniphest Tasks: T13515

Differential Revision: https://secure.phabricator.com/D21141
2020-04-19 09:01:07 -07:00
epriestley
3963c86ad5 Pass timeline view data to comment previews, restoring Differential comment previews
Summary:
Ref T13222. In D19918, I refactored how timelines get "view data". Today, this is always additional data about which images/changesets/diffs are visible on the current revision/commit/mock, so we can tell if inline comments should be linked to a `#anchor` on the same page (if the inline is rendered there somewhere) or to a `/D123?id=1&vs=2` full link on a different page (if it isn't), but in general this could be any sort of state information about the current page that affects how the timeline should render.

Previously, comment previews did not use any specialized object code and always rendered a "generic" timeline story. This was actually a bug, but none of the code we have today cares about this (since it's all inline related, and inlines render separately) so it never impacted anything.

After the `TimelineEngine` change, the preview renders with Differential-specific code. This is more correct, but we were not passing the preview the "view data" so it broke.

This preview doesn't actually need the view data and we could just make it bail out if it isn't present, but pass it through for consistency and so this works like we'd expect if we do something fancier with view data in the future.

Test Plan: Viewed comment and inline comment previews in Differential, saw old behavior restored.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D19943
2019-01-03 13:06:54 -08:00
epriestley
6c43d1d52c Remove "willRenderTimeline()" from ApplicationTransactionInterface
Summary:
Depends on D19914. Ref T11351. Some of the Phoilo rabbit holes go very deep.

`PhabricatorApplicationTransactionInterface` currently requires you to implement `willRenderTimeline()`. Almost every object just implements this as `return $timeline`; only Pholio, Diffusion, and Differential specialize it. In all cases, they are specializing it mostly to render inline comments.

The actual implementations are a bit of a weird mess and the way the data is threaded through the call stack is weird and not very modern.

Try to clean this up:

  - Stop requiring `willRenderTimeline()` to be implemented.
  - Stop requiring `getApplicationTransactionViewObject()` to be implemented (only the three above, plus Legalpad, implement this, and Legalpad's implementation is a no-op). These two methods are inherently pretty coupled for almost any reasonable thing you might want to do with the timeline.
  - Simplify the handling of "renderdata" and call it "View Data". This is additional information about the current view of the transaction timeline that is required to render it correctly. This is only used in Differential, to decide if we can link an inline comment to an anchor on the same page or should link it to another page. We could perhaps do this on the client instead, but having this data doesn't seem inherently bad to me.
  - If objects want to customize timeline rendering, they now implement `PhabricatorTimelineInterface` and provide a `TimelineEngine` which gets a nice formal stack.

This leaves a lot of empty `willRenderTimeline()` implementations hanging around. I'll remove these in the next change, it's just going to be deleting a couple dozen copies of an identical empty method implementation.

Test Plan:
  - Viewed audits, revisions, and mocks with inline comments.
  - Used "Show Older" to page a revision back in history (this is relevant for "View Data").
  - Grepped for symbols: willRenderTimeline, getApplicationTransactionViewObject, Legalpad classes.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T11351

Differential Revision: https://secure.phabricator.com/D19918
2018-12-20 14:55:07 -08:00
epriestley
44c32839a6 When you "Request Review" of a draft revision, change the button text from "Submit Quietly" to "Publish Revision"
Summary:
See PHI975. Ref T13216. Ref T2543. Previously, see D19204 and PHI433.

When you're acting on a draft revision, we change the button text to "Submit Quietly" as a hint that your actions don't generate notifications yet.

However, this isn't accurate when one of your actions is "Request Review", which causes the revision to publish.

Allow actions to override the submit button text, and make the "Request Review" action change the button text to "Publish Revision".

The alternative change I considered was to remove the word "Quietly" in all cases.

I'm not //thrilled// about how complex this change is to adjust one word, but the various pieces are all fairly clean individually. I'm not sure we'll ever be able to use it for anything else, but I do suspect that the word "Quietly" was the change in D19204 with the largest effect by far (see T10000).

Test Plan:
  - Created a draft revision. Saw "Submit Quietly" text.
  - Added a "Request Review" action, saw it change to "Publish Revision".
  - Reloaded page, saw stack saved and "Publish Revision".
  - Removed action, saw "Submit Quietly".
  - Repeated on a non-draft revision, button stayed put as "Submit".
  - Submitted the various actions, saw them have the desired effects.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13216, T2543

Differential Revision: https://secure.phabricator.com/D19810
2018-11-15 20:50:21 -08:00
epriestley
185e72c881 Add aural section headers for "Event Timeline", "Add Comment", and "Comment Preview"
Summary: See PHI871. Ref T13197. These sections are only divided visually and don't have textual headers. Add aural headers.

Test Plan: {F5875471}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13197

Differential Revision: https://secure.phabricator.com/D19654
2018-09-11 13:30:10 -07:00
epriestley
75a5dd8d8c Add more accessibility labels for screen readers
Summary:
Depends on D19594. See PHI823. Ref T13164.

  - Add a label for the "X" button in comment areas, like "Remove Action: Change Subscribers".
  - Add a label for the floating header display options menu in Differential.
  - Add `role="button"` to `PHUIButtonView` objects that we render with an `<a ...>` tag.

Test Plan:
Viewed a revision with `?__aural__=true`:

  - Saw "Remove Action: ..." label.
  - Saw "Display Options" label.
  - Used inspector to verify that some `<a class="button" ...>` now have `<a class="button" role="button" ...>`. This isn't exhaustive, but at least improves things. A specific example is the "edit", "reply", etc., actions on inline comments.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13164

Differential Revision: https://secure.phabricator.com/D19595
2018-08-17 13:31:51 -07:00
epriestley
b7843da963 Don't expand folded timelines just because users went to any anchor whatsoever
Summary:
Ref T12970. See PHI43. Currently, the "Show Older Comments" link gets auto-clicked if the user visits **any** anchor. This is not correct.

Instead, only auto-click it if the user visits a numeric anchor. This fixes the behavior approximately 98% of the time. See T12970 for a followup on the remaining ambiguous cases.

Test Plan:
  - Viewed a revision with some folded transactions and a "Show Older Comments" link.
  - Clicked a link to a file in the table of contents, with a hash like `#1234abcd`.
    - Before: Timeline expanded and I ended up somewhere bad.
    - After: Timeline no longer expanded.
  - Manually changed hash to `#1234` (purely numeric), saw timeline expand.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12970

Differential Revision: https://secure.phabricator.com/D18458
2017-08-23 14:52:39 -07:00
epriestley
6c46f27d98 Add quest objectives to the minimap
Summary:
Add important objectives (like waygates and quest markers) to the minimap.

This also probably fixes @cspeckmim's bug with the {key @} keyboard shortcut.

Test Plan:
(This is probably easier to undestand if you `arc patch` + click around.)

{F4966037}

Reviewers: chad, amckinley

Reviewed By: chad

Subscribers: cspeckmim

Differential Revision: https://secure.phabricator.com/D17955
2017-05-19 12:01:01 -07:00
epriestley
e187519f00 When a transaction has no quote ref, render "@user wrote:" properly
Summary:
Fixes T12576. In Javascript, `data.ref` is null, which is getting turned into `/quote/?ref=null`.

The code already handles this case, just not with `ref=null` happening in JS:

https://secure.phabricator.com/source/phabricator/browse/master/src/applications/transactions/controller/PhabricatorApplicationTransactionCommentQuoteController.php;b54adc6161c205e146fabb801ca53a44d94da444$47-52

Test Plan:
{F4913862}

  - Also quoted a normal comment on a normal object in a normal way.

Reviewers: amckinley, chad

Reviewed By: chad

Maniphest Tasks: T12576

Differential Revision: https://secure.phabricator.com/D17720
2017-04-18 09:51:53 -07:00
epriestley
f8cebdc418 Make Differential inline events actually trigger comment preview refreshes
Summary:
Earlier, I made some changes so that when you create or edit an inline, the comment at the bottom of the page updates (even though you didn't fiddle with the stacked actions inputs).

At the last second I broke them by spelling this wrong while cleaning things up, so they didn't actually work. Spell the property correctly ("showPreview", not "shouldPreview").

Also, we have some JS which rewrites "Not Visible" into "View", but it fires in an inconvenient way now and is flickery for me. Ideally this should get cleaned up slightly better eventualy, but at least make is stop doing so much flickery layout for now.

Test Plan:
  - Wrote no comment on a revision.
  - Added an inline.
  - Saw comment preview properly update immediately.

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D17229
2017-01-19 12:32:49 -08:00
epriestley
4b94ae15e0 Straighten out some stacked action removal behaviors
Summary: Fixes T12065. Some of the normal behavior of these actions got juggled when I made "Accept" and "Reject" conflict.

Test Plan:
  - Added "Accept".
  - Added "Reject", saw it remove "Accept".
  - Added "Change Projects".
  - Added "Assign/Claim".
  - Removed "Assign/Claim".
  - Removed "Accept".

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12065

Differential Revision: https://secure.phabricator.com/D17143
2017-01-04 19:16:58 -08:00
epriestley
35750b9c61 Make some Differential comment actions (like "Accept" and "Reject") conflict with one another
Summary:
Ref T11114. When a user selects "Accept", and then selects "Reject", remove the "Accept". It does not make sense to both accept and reject a revision.

For now, every one of the "actions" conflicts: accept, reject, resign, claim, close, commandeer, etc, etc. I couldn't come up with any combinations that it seems like users are reasonably likely to want to try, and we haven't received combo-action requests in the past that I can recall.

Test Plan:
  - Selected "Accept", then selected "Reject". One replaced the other.
  - Selected "Accept", then selected "Change Subscribers". Both co-existed happily.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17132
2017-01-02 13:25:12 -08:00
epriestley
00313094d3 Make "View" links on Differential inline comment previews work again
Summary:
Ref T11114. Recent changes broke the links to jump to inline comments from the previews because they get hooked up with JS.

Restore the linking behavior.

Test Plan: Clicked "View" on an inline comment preview, jumped to that comment.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17131
2017-01-02 13:24:02 -08:00
epriestley
18249b097f Make inline comment preview and submission mostly work on EditEngine
Summary: Ref T11114. This comments nearly working on EditEngine. Only significant issue I caught is that the "View" link doesn't render properly because it depends on JS which is tricky to hook up. I'll clean that up in a future diff.

Test Plan: {F2279201}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17116
2016-12-31 10:10:29 -08:00
Chad Little
d78802f3ab Redesign Comment Box
Summary: Redesign the action comment box for better use in two column, mobile, nuance.

Test Plan: Test in mobile / desktop / tablet, adding and removing actions. Actionless comment boxes, etc.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: cspeckmim, Korvin

Differential Revision: https://secure.phabricator.com/D16811
2016-11-09 10:36:25 -08:00
epriestley
74d5ae7bec Clean up some log spam caused by races in VersionedDraft
Summary:
Two minor issues that I caught in the log while fixing Phame permissions:

  - We had a JS bug which would cause us to immediately generate two comment previews at the exact same time -- one for loading the page, and one for "switching to desktop". Instead, only generate the "switch to desktop" preview if we really switched to desktop from a different device layout.
  - These two requests could end up reading/writing the VersionedDraft table at exactly the same time fairly often (e.g., after a comment submission, the page would load, send two preview requests at exactly the same time, and they'd race fairly reliably for me locally). If we do race, recover from the race.

Test Plan:
Submitted some Phame comments.

  - No more error log errors about VersionedDraft keys.
  - Saw only one preview request when loading the page instead of two.

Here's the specific stack trace I caught:

```
[Mon Sep 05 12:15:33.639930 2016] [:error] [pid 50608] [client 127.0.0.1:55278] [2016-09-05 14:15:33] EXCEPTION: (AphrontDuplicateKeyQueryException) #1062: Duplicate entry 'PHID-POST-fknnpzjnsdgc3rqobhst-PHID-USER-pr5rjpuilpfserepsd2k-13' for key 'key_object' at [<phutil>/src/aphront/storage/connection/mysql/AphrontBaseMySQLDatabaseConnection.php:314]
[Mon Sep 05 12:15:33.640801 2016] [:error] [pid 50608] [client 127.0.0.1:55278] arcanist(head=master, ref.master=9e82ef979e81), corgi(head=master, ref.master=5b9171222bc9), instances(head=stable, ref.master=485bc8128198, ref.stable=2983bc917601), ledger(head=master, ref.master=4da4a24b8779), libcore(), phabricator(head=phame2, ref.master=4b6da9735ba7, ref.phame2=4b6da9735ba7), phutil(head=stable, ref.master=97f05269fdb1, ref.stable=c14343ee620e), services(head=stable, ref.master=1fcb5cdb7582, ref.stable=2d8088a5b4b3)
[Mon Sep 05 12:15:33.640815 2016] [:error] [pid 50608] [client 127.0.0.1:55278]   #0 <#2> AphrontBaseMySQLDatabaseConnection::throwCommonException(integer, string) called at [<phutil>/src/aphront/storage/connection/mysql/AphrontBaseMySQLDatabaseConnection.php:348]
[Mon Sep 05 12:15:33.640830 2016] [:error] [pid 50608] [client 127.0.0.1:55278]   #1 <#2> AphrontBaseMySQLDatabaseConnection::throwQueryCodeException(integer, string) called at [<phutil>/src/aphront/storage/connection/mysql/AphrontBaseMySQLDatabaseConnection.php:289]
[Mon Sep 05 12:15:33.640833 2016] [:error] [pid 50608] [client 127.0.0.1:55278]   #2 <#2> AphrontBaseMySQLDatabaseConnection::throwQueryException(mysqli) called at [<phutil>/src/aphront/storage/connection/mysql/AphrontBaseMySQLDatabaseConnection.php:185]
[Mon Sep 05 12:15:33.640836 2016] [:error] [pid 50608] [client 127.0.0.1:55278]   #3 <#2> AphrontBaseMySQLDatabaseConnection::executeRawQuery(string) called at [<phutil>/src/xsprintf/queryfx.php:8]
[Mon Sep 05 12:15:33.640839 2016] [:error] [pid 50608] [client 127.0.0.1:55278]   #4 <#2> queryfx(AphrontMySQLiDatabaseConnection, string, string, string, array, string)
[Mon Sep 05 12:15:33.640841 2016] [:error] [pid 50608] [client 127.0.0.1:55278]   #5 <#2> call_user_func_array(string, array) called at [<phutil>/src/aphront/storage/connection/AphrontDatabaseConnection.php:42]
[Mon Sep 05 12:15:33.640844 2016] [:error] [pid 50608] [client 127.0.0.1:55278]   #6 <#2> AphrontDatabaseConnection::query(string, string, string, array, string) called at [<phabricator>/src/infrastructure/storage/lisk/LiskDAO.php:1261]
[Mon Sep 05 12:15:33.640846 2016] [:error] [pid 50608] [client 127.0.0.1:55278]   #7 <#2> LiskDAO::insertRecordIntoDatabase(string) called at [<phabricator>/src/infrastructure/storage/lisk/LiskDAO.php:1106]
[Mon Sep 05 12:15:33.640849 2016] [:error] [pid 50608] [client 127.0.0.1:55278]   #8 <#2> LiskDAO::insert() called at [<phabricator>/src/infrastructure/storage/lisk/LiskDAO.php:1075]
[Mon Sep 05 12:15:33.640851 2016] [:error] [pid 50608] [client 127.0.0.1:55278]   #9 <#2> LiskDAO::save() called at [<phabricator>/src/applications/draft/storage/PhabricatorVersionedDraft.php:65]
[Mon Sep 05 12:15:33.640854 2016] [:error] [pid 50608] [client 127.0.0.1:55278]   #10 <#2> PhabricatorVersionedDraft::loadOrCreateDraft(string, string, integer) called at [<phabricator>/src/applications/transactions/editengine/PhabricatorEditEngine.php:1669]
[Mon Sep 05 12:15:33.640857 2016] [:error] [pid 50608] [client 127.0.0.1:55278]   #11 <#2> PhabricatorEditEngine::buildCommentResponse(PhamePost) called at [<phabricator>/src/applications/transactions/editengine/PhabricatorEditEngine.php:894]
[Mon Sep 05 12:15:33.640859 2016] [:error] [pid 50608] [client 127.0.0.1:55278]   #12 <#2> PhabricatorEditEngine::buildResponse() called at [<phabricator>/src/applications/phame/controller/post/PhamePostEditController.php:60]
[Mon Sep 05 12:15:33.640862 2016] [:error] [pid 50608] [client 127.0.0.1:55278]   #13 <#2> PhamePostEditController::handleRequest(AphrontRequest) called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:237]
[Mon Sep 05 12:15:33.640865 2016] [:error] [pid 50608] [client 127.0.0.1:55278]   #14 phlog(AphrontDuplicateKeyQueryException) called at [<phabricator>/src/aphront/handler/PhabricatorAjaxRequestExceptionHandler.php:27]
[Mon Sep 05 12:15:33.640868 2016] [:error] [pid 50608] [client 127.0.0.1:55278]   #15 PhabricatorAjaxRequestExceptionHandler::handleRequestException(AphrontRequest, AphrontDuplicateKeyQueryException) called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:644]
[Mon Sep 05 12:15:33.640870 2016] [:error] [pid 50608] [client 127.0.0.1:55278]   #16 AphrontApplicationConfiguration::handleException(AphrontDuplicateKeyQueryException) called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:242]
[Mon Sep 05 12:15:33.640873 2016] [:error] [pid 50608] [client 127.0.0.1:55278]   #17 AphrontApplicationConfiguration::processRequest(AphrontRequest, PhutilDeferredLog, AphrontPHPHTTPSink, MultimeterControl) called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:149]
[Mon Sep 05 12:15:33.640879 2016] [:error] [pid 50608] [client 127.0.0.1:55278]   #18 AphrontApplicationConfiguration::runHTTPRequest(AphrontPHPHTTPSink) called at [<phabricator>/webroot/index.php:17]
```

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D16494
2016-09-05 13:01:53 -07:00
Ben Schmidt
dea952aa48 Make links to hidden comments work (Fixes T11117) 2016-07-09 18:07:37 +10:00
epriestley
ad7239d64c Don't show continuation lines in "stacked action" comment previews
Summary:
Ref T10698. Currently, we show the spacer/continuation lines around previews, but these don't make sense in previews.

(Other stuff also uses this code so I can't simply remove `spacer`.)

Test Plan:
Before:

{F1199924}

After:

{F1199925}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10698

Differential Revision: https://secure.phabricator.com/D15562
2016-04-01 08:14:45 -07:00
epriestley
5a723bff91 Support synthetic submits from stacked actions
Summary: Fixes T10116.

Test Plan:
  - Prior to patch: Control/Command + Enter submitted form but ignored actions.
  - After patch: verifid command + enter submits actions.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10116

Differential Revision: https://secure.phabricator.com/D14991
2016-01-11 09:16:53 -08:00
epriestley
f422c9b955 Allow users to swipe away stacked actions on mobile
Summary:
Ref T10004. This might be a little too funky or magical, but is probably not necessary too often and easier than doing new layout with the CSS.

Also make the desktop icons a little meatier.

Test Plan: Swiped things away on mobile. Clicked things away on desktop.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10004

Differential Revision: https://secure.phabricator.com/D14857
2015-12-22 19:17:11 -08:00
epriestley
8752bd4966 Disable live previews on mobile
Summary:
Fixes T1895. Now that we have on-demand prviews, we can use them on mobile. On mobile:

  - don't show live previews;
  - only save drafts every 10 seconds.

Also, show fewer remarkup buttons on mobile to try to make sure the more important ones (preview, e.g.) fit.

Test Plan:
  - Made window narrower and wider to trigger preview/no-preview behavior.
  - Used DarkConsole to verify request rate.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T1895

Differential Revision: https://secure.phabricator.com/D14856
2015-12-22 12:52:51 -08:00
epriestley
d7693a93b3 Provide "Change Projects" and "Change Subscribers" (instead of "Add ...") in comment actions
Summary:
Ref T9908. Fixes T6205.

This is largely some refactoring to improve the code. The new structure is:

  - Each EditField has zero or one "submit" (normal edit form) controls.
  - Each EditField has zero or one "comment" (stacked actions) controls.
    - If we want more than one in the future, we'd just add two fields.
  - Each EditField can have multiple EditTypes which provide Conduit transactions.
  - EditTypes are now lower-level and less involved on the Submit/Comment pathways.

Test Plan:
  - Added and removed projects and subscribers.
  - Changed task statuses.
  - In two windows: added some subscribers in one, removed different ones in the other. The changes did not conflict.
  - Applied changes via Conduit.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T6205, T9908

Differential Revision: https://secure.phabricator.com/D14789
2015-12-15 15:03:34 -08:00
epriestley
59ae0d6fff Allow EditEngine create and edit forms to be reordered
Summary:
Ref T9132. Ref T9908. Puts reordering UI in place:

  - For create forms, this just lets you pick a UI display order other than alphabetical. Seems nice to have.
  - For edit forms, this lets you create a hierarchy of advanced-to-basic forms and give them different visibility policies, if you want.

Test Plan:
{F1017842}

  - Verified that "Edit Thing" now takes me to the highest-ranked edit form.
  - Verified that create menu and quick create menu reflect application order.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9132, T9908

Differential Revision: https://secure.phabricator.com/D14704
2015-12-08 13:00:54 -08:00
epriestley
273e22d59f Save stacked actions in drafts, not just comments
Summary:
Ref T9132. Fixes T4580. Thhat might actually have been fixed a while ago or something since it describes a buggy/bad interaction which doesn't reproduce for me at HEAD.

This saves and restores all the stacked actions (subscribers, projects, etc) so that you don't lose anything if you close a window by accident.

Test Plan:
Added a bunch of actions in various states, reloaded the page, draft stuck around.

Submitted form, actions didn't stick around anymore.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T4580, T9132

Differential Revision: https://secure.phabricator.com/D14675
2015-12-04 16:29:43 -08:00
epriestley
eded19a5c6 Unify EditEngine preview behavior; prepare for saving complex drafts
Summary:
Ref T9132. We currently have an old preview/draft behavior and a new actions behavior.

Let the actions behavior do drafts/previews too, so we can eventually throw away the old thing.

This is pretty much just copying the old behavior into the new one, but with a few tweaks. The major change is that we submit all the stacked actions behavior now, so the preview reflects everything the change will do (and, soon, we can save it in the draft in a consistent way).

Also includes one hack-fix that I'll clean up at some point.

Test Plan: Added a bunch of stacked actions and observed meaningful previews.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9132

Differential Revision: https://secure.phabricator.com/D14672
2015-12-04 16:29:40 -08:00
epriestley
92ea07e787 Restore "Change Status" and "Change Priority" comment actions to Maniphest
Summary: Ref T9132. Supports selects in stacked actions and adds "Change Status" + "Change Priority".

Test Plan: Changed status and priority from stacked actions.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9132

Differential Revision: https://secure.phabricator.com/D14667
2015-12-04 16:29:33 -08:00
epriestley
8bbea6d41c Make "Add Action..." add actions at the bottom instead of the top
Summary: Ref T9132. Shhh this never happened shhhhhhh.

Test Plan: Selected multiple actions, saw them add at the bottom.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9132

Differential Revision: https://secure.phabricator.com/D14664
2015-12-04 16:29:28 -08:00
epriestley
dc0d914134 Basic stacked action support for EditEngine
Summary: Ref T9132. This still has a lot of rough edges but the basics seem to work OK.

Test Plan: {F1012627}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9132

Differential Revision: https://secure.phabricator.com/D14653
2015-12-03 12:32:02 -08:00
epriestley
9aee90f8c1 Allow form configurations to retitle and reorder forms and add preambles
Summary:
Ref T9132. This just makes edited forms do //something//, albeit not anything very useful yet.

You can now edit a form and:

  - Retitle it;
  - add a preamble (instructions on top of the form); and
  - reorder the form's fields.

Test Plan:
{F974632}

{F974633}

{F974634}

{F974635}

{F974636}

Reviewers: chad

Reviewed By: chad

Subscribers: hach-que

Maniphest Tasks: T9132

Differential Revision: https://secure.phabricator.com/D14503
2015-11-22 15:12:57 -08:00
epriestley
107d6db1e8 Don't put a spacer line on the bottom of previews
Summary: Fixes T8182. See screenshot in that task. We currently render a line to nowhere at the bottom of these previews. Instead, only render a line at the top.

Test Plan:
{F409078}

Also looked at a couple other applications that use this and they looked correct.

Reviewers: btrahan, lpriestley, chad

Reviewed By: chad

Subscribers: epriestley

Maniphest Tasks: T8182

Differential Revision: https://secure.phabricator.com/D12905
2015-05-18 12:18:10 -07:00
epriestley
eaa883cf37 Fix anchor-clicking scroll positions
Summary:
Fixes T7069. When jumping to a comment anchor, we get the scroll positions wrong.

Partly this is fixing some calcaulations; partly, the "show older comments" and "scroll anchor" stuff were fighting over the scroll position. Since the anchor can take care of things on its own, just let it handle stuff.

Test Plan:
  - Clicked comment anchors.
  - Loaded pages with anchors in the URI.
  - Loaded pages with anchors hidden behind "show older comments".

In all cases, got the right scroll position.

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley

Maniphest Tasks: T7069

Differential Revision: https://secure.phabricator.com/D11540
2015-01-28 08:26:10 -08:00
Bob Trahan
3a85d0bf5a Transactions - change show all key from "~" to "@"
Summary: (some) international keyboard layouts can not type "~" in a way as to trigger this so use "@" instead. Save the suggested "+" as that seems like it would be useful for some future "adding stuff" keyboard workflow. Pretty stoked to get this squared away as I am quite confident our unreleased product will now be a huge smashing success. Ref T6683.

Test Plan: made sure my choice was okay via https://en.wikipedia.org/wiki/Dead_key#Dead_keys_on_various_keyboard_layouts; used the "@" key to show all transactions

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T6683

Differential Revision: https://secure.phabricator.com/D10983
2014-12-12 11:23:56 -08:00
Bob Trahan
9b865f18e8 Transactions - make "show older" area bigger and include keyboard command
Summary: Fixes T6683.

Test Plan: clicked the yellow box and it worked! pressed '~' and it worked!

Reviewers: epriestley, chad

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T6683

Differential Revision: https://secure.phabricator.com/D10932
2014-12-10 12:12:38 -08:00
Bob Trahan
6ab3f06b6e Transactions - adding willRenderTimeline to handle tricky cases
Summary: Fixes T6693.

Test Plan:
Made a bunch of comments on a diff with differential, being sure to leave inlines here and there. This reproduced the issue in T6693. With this patch this issue no longer reproduces!

Successfully "showed older changes" in Maniphest too.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T6693

Differential Revision: https://secure.phabricator.com/D10931
2014-12-04 13:58:52 -08:00
Bob Trahan
d6341cfffe Transactions - add pagination to application transactions
Summary: Ref T4712. This adds pagination. Future diffs will need to deploy `buildTransactionTimeline` everywhere and massage this stuff as necessary if we hit any special cases.

Test Plan: Set page size to "5" to make it need to paginate often. Verified proper transactions loaded in and the javascript actions worked.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T4712

Differential Revision: https://secure.phabricator.com/D10887
2014-12-02 13:10:29 -08:00
Bob Trahan
27d44594dc Transactions - add "view raw" action
Summary: Use cutlery icon for hilarity. Ref T5768.

Test Plan: made something with remarkup in it, used 'view raw' and saw the remarkup raw in a nice little dialogue.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T5768

Differential Revision: https://secure.phabricator.com/D10183
2014-08-07 15:21:32 -07:00
Joshua Spence
79f9d87401 Change double quotes to single quotes in JavaScript.
Summary: As advised by JSHint.

Test Plan: I'm not really sure how to comprehensively test this. It looks okay to me.

Reviewers: chad, epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D9677
2014-06-24 03:35:39 +10:00
Joshua Spence
a0d63322b1 Remove unused JavaScript variables.
Summary: There are a bunch of unused variables in JavaScript files. These were identified with JSHint.

Test Plan: It's pretty hard to test this thoroughly... on inspection, it seems that everything //should// be okay (unless we are doing weird things with the JavaScript).

Reviewers: chad, #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D9676
2014-06-24 03:27:47 +10:00
epriestley
9f2efd0fa0 Remove ajaxey comment magic in Pholio, and also some bugs
Summary:
Fixes T5424.

  - One concrete issue: drafts were not being cleared properly because `__draft__` was not set on submission. This (mostly) fixes phantom drafts.
  - This ajax comment magic feels weird and floaty and generally has problems. For example, if you add subscribers or inlines, all the stuff on the page which represents those won't update automatically. Instead, just reload. Maybe we'll ajax this stuff some day, but it feels like a net negative for now.
  - Also remove it from other applications where it's currently used.
  - Fix an issue with inline previews.

Test Plan: Made some comments on a mock, everything worked normally like I expected it to.

Reviewers: chad

Reviewed By: chad

Subscribers: epriestley

Maniphest Tasks: T5424

Differential Revision: https://secure.phabricator.com/D9649
2014-06-21 12:50:40 -07:00
epriestley
bfc1ccfdf1 Move all comment management junk into a dropdown menu
Summary:
man I sure hate Javascript

I removed the ajax-edit and ajax-remove interactions, becuase they were prohibitively complex to get working given that the entire menu has to change too. Instead, the page just reloads. This works perfectly fine in practice.

If we want to restore these in the future, we should have the server re-render the entire transaction group or something. I think very little is lost here, though.

Test Plan:
  - Took all the actions.
  - Used existing dropdown menus.

{F150196}

Reviewers: chad, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D8966
2014-05-05 10:57:23 -07:00
epriestley
707c5aec81 Add a quote action to Differential and Maniphest
Summary:
Ref T4119. This is ugly for now, but technically works.

The comment area and transaction log don't realy know about each other, so for the moment the linking is a bit manual. Differential/Maniphest are special cases anyway.

Test Plan: {F149992}

Reviewers: chad, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4119

Differential Revision: https://secure.phabricator.com/D8957
2014-05-05 10:55:58 -07:00
epriestley
58f66fea80 Allow users to remove their own comments, and administrators to remove any comment
Summary:
Fixes T4909. Adds a "remove" link next to the edit link, which permanently hides a comment. Addresses two use cases:

  - Allowing administrators to clean up spam.
  - Allowing users to try to put the genie back in the bottle if they post passwords or sensitive links, etc.

The user who removed the comment is named in the removal text to enforce some level of administrative accountability.

No data is deleted, but there's currently no method to restore these comments. We'll see if we need one.

This is cheating a little bit by storing "removed" as "2" in the isDeleted field. This doesn't seem tooooo bad for now.

Test Plan:
  - Removed some of my comments.
  - As an administrator, removed other users' comments.
  - Failed to view history of a removed comment.
  - Failed to edit a removed comment.
  - Failed to remove a removed comment.
  - Verified feed doesn't show the old comment after comment removal.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: qgil, chad, epriestley

Maniphest Tasks: T4909

Differential Revision: https://secure.phabricator.com/D8945
2014-05-05 10:55:32 -07:00
Bob Trahan
496a7d8967 Transactions - make edit transactions that are grouped work nicely
Summary: ...the key is to move a layer lower and beam down the updated comment. There is a wee bit of Javascript gymnastics going on here. Fixes T4608.

Test Plan: made a comment + resolve. clicked edit and made changes. noted transaction updated correctly and "history" link worked. edited again to a deletion and noted the "this is deleted" looked right and history link still worked

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin, chad

Maniphest Tasks: T4608

Differential Revision: https://secure.phabricator.com/D8702
2014-04-04 12:23:22 -07:00
epriestley
88227d26bc Allow CustomField to provide ApplicationTransaction change details
Summary:
Ref T3886. Ref T418. For fields like "Summary" and "Test Plan" where changes can't be summarized in one line, allow CustomField to provide a "(Show Details)" link and render a diff.

Also consolidate some of the existing copy/paste, and simplify this featuer slightly now that we've move to dialogs.

Test Plan:
{F115918}

  - Viewed "description"-style field changes in phlux, pholio, legalpad, maniphest, differential, ponder (questions), ponder (answers), and repositories.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3886, T418

Differential Revision: https://secure.phabricator.com/D8284
2014-02-21 11:53:04 -08:00
Bob Trahan
5729bbc085 Fix transaction comment bug
Summary: form.reset() resets a form to whatever values were present when the form was loaded into the DOM. Instead, grab all the pertinent form bits and set there values to "clear". I don't think there's too much utility in putting this somewhere more general, but it could be something like DOM.clearForm(form) or something. Fixes T3629.

Test Plan:
repro'd original issue

 - open legalpad doc (or your favorite application transaction powered app)
 - type in a comment but do not submit (you are creating a draft) e.g. "foo"
 - reload page and note comment appears e.g. "foo"
 - change comment e.g. "foobar"
 - submit comment
 - BUG - after submission, the comment reverts to the comment at initial page load e.g. "foo"

then after this patch, I can't repro it anymore with these steps - the comment is correctly blank

 NOTE: this will need to be tested with more complicated forms.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T3629

Differential Revision: https://secure.phabricator.com/D8220
2014-02-13 15:10:20 -08:00
Bob Trahan
320498d3d0 Transactions - make the details stuff generic and ajaxy
Summary: Fixes T2213

Test Plan: Updated a pholio mock description. Observed that when I first showed details there was a round trip made. Toggled show / hide noting no more trips made to server.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, aran

Maniphest Tasks: T2213

Differential Revision: https://secure.phabricator.com/D6801
2013-08-22 16:45:14 -07:00
epriestley
cf9dc5d189 Fix bug when multiple comment forms appear on a single page
Summary:
Ref T3373. The submit listener doesn't properly scope the form it listens to right now, so several forms on the page mean that comments post to one of them more or less at random.

Scope it properly by telling it which object PHID it is associated with.

Test Plan: Made Question comments, saw comments Ajax in on the question itself rather than on an arbitrary answer.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3373

Differential Revision: https://secure.phabricator.com/D6611
2013-07-29 12:04:10 -07:00
epriestley
4be9ccaea8 Restore comments on Ponder answers
Summary:
Ref T3373. This is still pretty messy:

  - The JS bugs out a bit with multiple primary object PHIDs on a single page. I'll fix this in a followup.
  - The comment form itself is enormous, I'll restore some show/hide stuff in a followup.

Test Plan: Added answer comments in Ponder.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3373

Differential Revision: https://secure.phabricator.com/D6608
2013-07-29 12:04:07 -07:00
Bob Trahan
6020e213e9 Make ApplicationTransactions preview -> submit flow less janky
Summary: ...also make it so in Pholio when you add an inline comment the preview refreshes. Fixes T2649.

Test Plan: played around in pholio leaving commentary. noted that a new inline comment would refresh the preview.

Reviewers: epriestley, chad

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T2649

Differential Revision: https://secure.phabricator.com/D6267
2013-06-24 15:41:59 -07:00
epriestley
0569218201 Use JsShrink if jsxmin is not available
Summary: If `jsxmin` is not available, use a pure PHP implementation instead (JsShrink).

Test Plan:
  - Ran `arc lint --lintall` on all JS and fixed every relevant warning.
  - Forced minification on and browsed around the site using JS behaviors. Didn't hit anything problematic.

Reviewers: vrana, btrahan

Reviewed By: vrana

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D5670
2013-05-18 17:04:22 -07:00