Summary:
Preload all the images when viewing a pholio. Fixes T2692.
We should probably use lower-resolution previews on mobile and only show full resolution when the user taps. We should also suppress the thumbnails from loading on mobile since they aren't visible. But neither is critical for the moment.
Test Plan: Added logging, verified everything loaded.
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2692
Differential Revision: https://secure.phabricator.com/D5316
Summary: Pholio inline comments now have minimum size of 16x16 px
Test Plan: Made some inlines
Reviewers: epriestley
CC: aran, Korvin, AnhNhan
Maniphest Tasks: T2647
Differential Revision: https://secure.phabricator.com/D5250
Summary:
Ref 2700. No sexy animation yet, but allows you to swipe left and right to switch photos. Generally feels pretty good to me.
Also fixes a left/right but and a bug where taps could be interpreted as gestures and simplifies some touch code.
derpdog
Test Plan:
Swiped left and right in Pholio.
{F35239}
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D5309
Summary:
Ref T2700. Allow JS to listen for swipes on devices.
There are a bunch of tricky cases here and I probably didn't get them all totally right, but this interaction broadly looks like this:
- We implement gesture recognition for the mouse in device modes (narrow browser), and for touch events from an actual device.
- The sigil `touchable` indicates that a node wants to react to touch events.
- When the user touches a `touchable` node, we start listening for moves. They might be tapping/clicking (in which case we don't care), but they might also be gesturing.
- Once the user moves their finger/pointer far enough away from the tap origin, we recognize it as a gesture. I hardcoded this at 20px; I wasn't able to find any "official" Apple value, but 20px seems like a common default.
- At this point, we look at where their finger has moved.
- If they moved it mostly up/down, we interpret the gesture as "scroll" and just stop listening. The device does its own thing.
- However, if they moved it mostly left/right, we interpret it as a "swipe". We start killing the moves so the device doesn't scroll.
- Once we've recognized that a gesture is underway, we send a "gesture.swipe.start" event and then "gesture.swipe.move" events for every move.
- When the user ends the gesture, we send "gesture.swipe.end".
- If the user cancels the gesture (currently, only by tapping with a second finger), we send "gesture.swipe.cancel".
- Gesture events have raw position data and some convenience fields.
Test Plan:
Wrote UI example and used it from the Desktop, iPhone simulator, and a real iphone.
- The code always seems to get "scroll" vs "swipe" correct (i.e., consistent with my intentions).
- The threshold feels pretty good to me.
- Tapping with a second finger cancels the action.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T2700
Differential Revision: https://secure.phabricator.com/D5308
Summary: Fixes T2711. Allows special keys (arrows, tab, return) to be bound as shortcuts. Binds left and right as shortcuts in Pholio.
Test Plan: Pressed left. Pressed right.
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2711
Differential Revision: https://secure.phabricator.com/D5303
Summary: files widget updates as new files are added. made basically all edits "ajax" except for when you change the conpherence image which just does a reload. I will fix this in a future diff but it was starting to spiral out of control a bit.
Test Plan: commented on a task with files and noted the updated file widget. updated header image via drag and drop and dialogue -- noted a full reload. cropped image and re-titled conpherence - ajax updated as expected.
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2530
Differential Revision: https://secure.phabricator.com/D5288
Summary: okay title. other apps can get this by implementing shouldAllowPublic and set(ting)RequestURI on TransactionsCommentView. note i put some css inline -- let me know if that belongs someplace else or needs better design.
Test Plan: viewed a mock logged out and saw new button. used new button and ended up on the mock logged in with a clean URI.
Reviewers: epriestley, chad
Reviewed By: chad
CC: aran, Korvin
Maniphest Tasks: T2653
Differential Revision: https://secure.phabricator.com/D5266
Summary:
I have blame enabled by default and displaying files with long history takes easily over 10 seconds.
Load the blame data by AJAX instead.
This is actually doing more work and the total response time is longer but it's worth it for me as I am interested just in the file contents quite often.
I know you were talking about building blame cache but until we have it...
I'm not sure if the AJAX loading indicator in bottom right corner is enough to inform the user that we are loading it on background.
Test Plan:
?view=highlighted
?view=plainblame
?view=blame
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5244
Summary: serving up for some feedback -- does anything fancy need to happen when new messages load (say highlight em and fade the highlight out) or just scrolling down okay? in JS I have a TODO about how come it doesn't work; that code works okay in my console if I print out various debugging values and enter them in manually.
Test Plan: pontificate with myself; note new message and message entry updates properly. pontificate as different user then as myself; note two messages load and message entry updates properly. pontificate as a user who isn't the last to reply; note new message and message entry updates properly
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2522
Differential Revision: https://secure.phabricator.com/D5214
Summary:
Ref T2641. Currently, we scale images to fit horizontally, but let them have arbitrary vertical size. This is nice in theory but kind of sucks in practice because it makes everything below the stage jump around when you switch images. It would also make swiping through images on mobile super weird.
Instead, scale to fit in both dimensions. This feels a lot better and more application-like to me. (I also think most mocks are not especially tall?)
Test Plan:
{F34648}
(Note that the image is enormous.)
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2641
Differential Revision: https://secure.phabricator.com/D5233
Summary:
Ref T2644. This isn't perfect, but makes Pholio more-or-less usable on mobile. In particular:
- Inline comments drop below the image.
- Clicking the image lightboxes a full size version so you can scroll around it and inspect details.
- Clicking it again dismisses it.
Things that aren't good:
- Can't add inline comments. This seems complicated and not critical.
- Can't easily figure out which inline comments go where since there's no hover. Maybe let you tap them? Not sure if we really need this.
- Thumbs are between image and image data. I'm planning to get rid of the thumbs and do swipe left/right instead.
- It would be nice to scroll the lightbox to center on the part of the image you tapped. I just thought of this, though.
Test Plan:
{F34640}
{F34641}
{F34642}
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2644
Differential Revision: https://secure.phabricator.com/D5232
Summary: I broke the code which shows which thumb is currently selected when I made the URLs fancy, by changing the container from `div` to `a`.
Test Plan: Verified current thumb is now highlighted.
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D5231
Summary:
Fixes T2669.
- Currently, making an inline comment does not focus the textarea. Instead, focus the textarea.
- Currently, the positioning is kind of buggy. Make it viewport-relative and put the dialog slightly below the inline reticle.
- Use `JX.Workflow` more and simplify some of the ajax stuff.
Test Plan: Created inline comments.
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2669
Differential Revision: https://secure.phabricator.com/D5229
Summary:
Some recent CSS change tweaked these out a bit. They were also sort of weird looking after fixing the double-draw thing:
- Make them work properly;
- make the implementation a bit simpler;
- make them clearer visually.
Also fix a bug where "border" and "fill" were accidentally reversed.
Test Plan:
{F34625}
(The highlight on the reticle is a little hard to pick out in the screenshot, but very obvious in practice.)
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D5228
Summary: Currently, if you click and drag in the checkered background, we'll create a 1px-wide or 1px-tall inline comment. Don't start inlines unless the user clicks on the image itself.
Test Plan: Clicked on and off the image.
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D5227
Summary: Fixes T2646. Title and description are placeholders. Styles on this are a bit rough.
Test Plan: {F34623}
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2646
Differential Revision: https://secure.phabricator.com/D5226
Summary: Fixes T2658. Map `/Mxx/yy/` to images in a mock. Use HTML5 history stuff to make it work nicely. This also makes right-click-open-in-new-tab work correctly.
Test Plan: Clicked thumbs in a mock. Right-clicked thumbs. Copy and pasted a URI. Used browser back button.
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2658
Differential Revision: https://secure.phabricator.com/D5222
Summary: Fixes T2657. Adds j/k for previous/next image. This is consistent with Differential. We could add left/right later but it needs a little work to make those available.
Test Plan: Mashed j/k, saw images switch.
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2657
Differential Revision: https://secure.phabricator.com/D5220
Summary:
Fixes T2643. Show a loading state (currently just making the image transparent, but we could do something fancier) when an image is loading.
Fixes T2648. Cleans up the double draws we were previously doing.
Makes thumbnails react to mousedown in addition to click so they feel a little snappier. Make them stop reacting to control click, command click, etc.
Test Plan: Used `setTimeout()` to simulate a 1s image load delay, switched between images.
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2643, T2648
Differential Revision: https://secure.phabricator.com/D5219
Summary:
For a single line, I can use the right click.
But for highlighting multiple lines, I have to wait for page reload.
It would be nice to track this in history but that's more involved.
This is actually maybe better behavior.
Test Plan: Clicked on the line link, dragged and dropped on it.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5172
Summary:
Highlight thumb for image user is currently viewing.
Highlight selection when user mouseovers it's inline(on side).
Test Plan: {F33990}
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2446
Differential Revision: https://secure.phabricator.com/D5134
Summary:
- For images that aren't too large to fit, keep the stage at about 85% of the viewport height. This prevents it from bouncing around as you switch between images, and generally makes it feel big and important like it should. When the stage is larger than images, we center them vertically.
- As the viewport is resized, shrink the stage and, if necessary, the images on it.
Test Plan:
{F33617}
{F33618}
Reviewers: chad, ljalonen
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D5089
Summary:
Currently, if an image is too wide for the viewport, we freak out. Instead, scale it down.
This means we must also scale down all the rectangles on it, which is why this is tricky. However, all the draw/load separation has made it reasonably straightforward.
We'll possibly need to add some kind of "view full size" thing. I'm planning to add an element which shows "85%" or whatever if it's currently scaled.
Test Plan:
Before:
{F33607}
After:
{F33608}
Reviewers: chad, ljalonen
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D5088
Summary:
Currently, we draw the selection immediately after it changes. Instead, update state and then draw out of state.
Also simplify and clean up a few things. Make all the inline endpoints return data in the same format.
Test Plan: Made various inline comments.
Reviewers: chad, ljalonen
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D5085
Summary: `image` has been replaced with `active_image`. `imageData` is from a long time ago, I think.
Test Plan: Verified nothing seems to be broken.
Reviewers: chad, ljalonen
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D5084
Summary:
Currently, we issue one Ajax request to get inline comments, and then draw them when they come back. This has some limitations:
- A user could quickly click image 1, and then click image 2. The request for image 1 may take longer to come back than image 2. In this case, we'd draw the image 2 inlines and then draw the image 1 inlines when that request arrived, resulting in image 2 shown with both image 1 and image 2 comments. This is hard to make happen in a sandbox because the requests are so fast, but prevent it by drawing only the currently visible image's inlines.
- Every time we want to draw inlines, we need to request them -- even if we've already loaded them! This allows us to draw them without reloading them (although we don't actually do this, yet). When we do, it will make it faster to switch between images you've aleady looked at (and we can do other optimizations).
Test Plan: Clicked between images, saw inlines draw correctly. Added a new inline. Moused over inlines.
Reviewers: chad, ljalonen
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D5083
Summary:
- Currently, we register this behavior every time we load comments. Instead, register it once.
- We have two very similar behaviors for over/out. Just use one that does the right thing based on the event type.
Test Plan: Waved my mouse over areas of the image with reticles, saw the correct comments highlight.
Reviewers: chad, ljalonen
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D5082
Summary:
Currently, we use two different methods to draw Mock main images:
- When the page first loads, we draw the image on the server with PHP.
- When the user clicks a thumbnail, we draw the image on the client with JS.
Since we can't reasonably get rid of the client version of this, move it all to the client. This paves the way for some future features, like:
- Doing some magic with stage sizing
- URIs that jump to a specific comment
- showing "loading" indicators while images are loading
- Loading lowsrc images first?
Test Plan: Loaded page, clicked thumbs.
Reviewers: ljalonen, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D5073
Summary: Show editor when user clicks edit button for inline comment.
Test Plan: Verified that editor is shown.
Reviewers: epriestley
CC: aran, Korvin
Maniphest Tasks: T2446
Differential Revision: https://secure.phabricator.com/D5030
Summary:
Rendering of inline comments has now been moved to PholioInlineCommentView controller.
Delete almost deletes and edit... well not so much, but replaced google.fi with amazing popup.
Test Plan: Verified that inline comments still show up. Verified that delete almost deletes.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2446
Differential Revision: https://secure.phabricator.com/D4994
Conflicts:
src/applications/pholio/controller/PholioInlineController.php
Summary:
Some transactions (like editing configuration values, task descriptions, or Conpherence images) can't be simply explained and need an additional larger element to show them fully (like a text diff).
Support change details like this in ApplicationTransactions. Implements the element in Config, so you can see changes.
Test Plan: {F32974}
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2213
Differential Revision: https://secure.phabricator.com/D4984
Summary: Fixed T2428 a little bit
Test Plan: On trial, only the last n transactions loaded as hardcoded in ConpherenceViewController.php. Button was rendered.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2428
Differential Revision: https://secure.phabricator.com/D4898
Summary:
Show drafts for users that made them.
Show inline comments beside image, highlights them when user mouseovers selection.
Allow users that can view mock to add inline comment instead of only allowing users that can edit mock to add inline comment.
Test Plan:
Verified that inline comments are shown beside image. Verified that only drafts for current user are shown. Verified that inline comment is
highlithed when user mouseovers their selection.
Reviewers: epriestley
CC: aran, Korvin
Maniphest Tasks: T2446
Differential Revision: https://secure.phabricator.com/D4929
Summary: If a page is profiled, add an "X-Phabricator-Profiler" header to all Ajax requests, and profile those too.
Test Plan: Profiled a page, checked Darkconsole, saw profiles for everything.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4885
Summary: Saved inline comments are now shown for images.
Test Plan: Verified that inline comments are loaded and shown.
Reviewers: epriestley
CC: aran, Korvin
Maniphest Tasks: T2446
Differential Revision: https://secure.phabricator.com/D4866
Summary:
The selection reticle in Pholio is functional but not very pretty right now. Make it look a little nicer.
- Using `box-sizing: border-box;` allows us to get rid of the `x - 1` and `y - 2` stuff.
- I draw the reticle with two elements: one mostly-transparent which creates a fill, and one fully opaque to create a strong dashed border.
Test Plan: {F31803}
Reviewers: ljalonen, chad
Reviewed By: chad
CC: aran, kchr
Differential Revision: https://secure.phabricator.com/D4836
Summary:
mainly, this adds the image cropper - yay!
- also removes the file image from the handle stuff I added in V1. now we do all this crazy photo stuff.
Test Plan:
- uploaded a photo by dragging to header and noted 120 x 80 showed up on reload
- uploaded a photo by dragging to edit dialogue spot and noted 120 x 80 showed up on reload
- cropped a photo - noted it cropped right
- cropped a photo again and again and again - seems like it crops okay
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2418, T2399
Differential Revision: https://secure.phabricator.com/D4790
Summary: Comment draft is now saved
Test Plan: Verified that draft is saved
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, chad
Maniphest Tasks: T2446
Differential Revision: https://secure.phabricator.com/D4831
Summary: i think the DOM changed in conpherence with the menu upgrades. just noticed that when you select a new conpherence the old one is not de-selected. this fixes it by updating the javascript to ascend one node higher and then use DOM.scry with the right data sigil to get the nodes
Test Plan: read some messages, noted only the one I was reading had the entry highlighted in the left.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2399
Differential Revision: https://secure.phabricator.com/D4819
Summary:
Fixes T2482. After D4792, menus have more formal structure, but previously we were just shoving some `<div>` into the middle of the thing. This no longer works correctly, since we end up with `<div class="nice-formal-div"><div></div>`.
Just put IDs on all the items we're going to show/hide instead so we don't have to render any half-tags.
Test Plan: Home page show/hide works again.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2482
Differential Revision: https://secure.phabricator.com/D4810
Summary:
this was originally "just" adding the icons like I had bundled into D4790. It morphed a bit though and does a few things
- adds the icons
- cleans up widget CSS generally a bit so scrolling always works
- phutil_tag -- probably was a bad idea but I wanted to play with it. I think its harder to not break conpherence when you land the branch now maybs. Still up for fixing it immediately post land though.
Test Plan: played with conphernece a bit. Used FF and Chrome to verify CSS was looking okay-ish.
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2399
Differential Revision: https://secure.phabricator.com/D4814
Summary:
I attempted to test every interface. I probably missed some stuff, but I at least have some level of confidence that the `phutil_tag` branch is fairly stable.
Fixed these issues:
[1] Fixed a Herald issue with object links in transcripts. Some actions return
links; this was previously hard-coded.
[2] DarkConsole refactoring created an issue where the "`" event handler registered too many times.
[3] Fixed a bug where `strlen($value)` was being checked, but fields may now return array(). Possibly we should implement phutil_is_empty_html() or similar.
[4] Fixed a undefined variable issue for image edit transactions.
[5] Fixed an issue with rendering participant transactions. This required phutil_safe_html() because `pht()` can't handle `array()` for `%s`.
[6] Fixed an issue where feed was entirely overescaped by reverting an overly ambitious render_tag -> tag.
[7] Fixed an issue with strict tables and inserting `''` instead of `0` into an integer column.
[8] Fixed an issue where • was shown escaped.
[9] Fixed an issue where "no data" was overescaped.
[10] Fixed an issue with strict tables and inserting `''` instead of `0` into an integer column.
[11] Fixed an issue with strict tables and inserting `''`.
[12] Fixed an issue with missing space after ":" for mini panels.
Encountered (but did not fix) these issues:
[X1] "e" works incorrectly on comments you are not allowed to edit. Did not fix.
[X2] Coverage currently interacts incorrectly with "--everything" for Phutil tests.
Test Plan:
- Viewed Differential.
- Created a diff via copy/paste.
- Viewed standalone diff.
- Jumped to diff via changeset table.
- Created a revision.
- Updated revision.
- Added a comment.
- Edited revision dependencies.
- Edited revision tasks.
- Viewed MetaMTA transcripts.
- Viewed Herald transcripts [1].
- Downloaded raw diff.
- Flagged / unflagged revision.
- Added/edited/deleted inline comment.
- Collapsed/expanded file.
- Did show raw left.
- Did show raw right.
- Checked previews for available actions.
- Clicked remarkup buttons
- Used filetree view.
- Used keyboard: F, j, k, J, K, n, p, t, h, "?" [2] [X1].
- Created a meme.
- Uploaded a file via drag and drop.
- Viewed a revision with no reviewers.
- Viewed a revision with >100 files.
- Viewed various other revisions [3].
- Viewed an image diff.
- Added image diff inline comments.
- Viewed Maniphest.
- Ran various queries.
- Created task.
- Created similar task.
- Added comments to tasks.
- Ran custom query.
- Saved custom query.
- Edited custom queries.
- Drag-reordered tasks.
- Batch edited tasks.
- Exported tasks to excel.
- Looked at reports (issue in T2311 notwithstanding).
- Viewed Diffusion.
- Browsed Git, SVN, HG repositories.
- Looked at history, browse, change, commit views.
- Viewed audit.
- Performed various audit searches.
- Viewed Paste.
- Performed paste searches.
- Created, edited, forked paste.
- Viewed Phriction.
- Edited a page.
- Viewed edit history.
- Used search typeahead to search for user / application.
- Used search to search for text.
- Viewed Phame.
- Viewed Blog, Post.
- Viewed live post.
- Published/unpublished post.
- Previewed post.
- Viewed Pholio.
- Edited/commented mock.
- Viewed ponder.
- Viewed question.
- Added answer/comment.
- Viewed Diviner.
- Viewed Conpherence [4] [5].
- Made Conpherence updates.
- Viewed calendar.
- Created status.
- Viewed status.
- Viewed Feed [6].
- Viewed Projects.
- Viewed project detail.
- Edited project.
- Viewed Owners.
- Viewed package detail.
- Edited package [7].
- Viewed flags.
- Edited flag.
- Deleted flag.
- Viewed Herald.
- Viewed rules.
- Created rule.
- Edited rule.
- Viewed edit log.
- Viewed transcripts.
- Inspected a transcript.
- Viewed People.
- Viewed list.
- Administrated user.
- Checked username/delete stuff.
- Looked at create/import LDAP/activity logs.
- Looked at a user profile.
- Looked at user about page.
- Looked at Repositories.
- Edited repository.
- Edited arcanist project.
- Looked at daemons.
- Looked at all daemons [8].
- Viewed combined log.
- Looked at configuration.
- Edited configuration.
- Looked at setup issues [9].
- Looked at current settings.
- Looked at application list.
- Installed / uninstalled applications [10].
- Looked at mailing lists.
- Created a mailing list.
- Edited a mailing list.
- Looked at sent mail.
- Looked at received mail.
- Looked at send/receive tests.
- Looked at settings.
- Clicked through all the panels.
- Looked at slowvote.
- Created a slowvote [11].
- Voted in a slowvote.
- Looked at Macro.
- Created a macro.
- Edited a macro.
- Commented on a macro.
- Looked at Countdown.
- Created a Countdown.
- Looked at it.
- Looked at Drydock.
- Poked around a bit.
- Looked at Fact.
- Poked around a bit.
- Looked at files.
- Looked at a file.
- Uploaded a file.
- Looked at Conduit.
- Made a Conduit call.
- Looked at UIExamples.
- Looked at PHPAST.
- Looked at PHIDs.
- Looked at notification menu.
- Looked at notification detail.
- Logged out.
- Logged in.
- Looked at homepage [12].
- Ran `arc unit --everything --no-coverage` [X2].
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4807
Summary:
See D4812.
- This preference disables the file tree completely.
- It defaults off, so users who want it will have to go turn it on.
- Maybe slightly cleaner would be doing this if the tree was hidden and then ajaxing it in if you press "F", but that's complicated and I don't want to bother.
- Generally, I think this element is useful to something like 5% of users and not useful to 95%.
Test Plan: Enabled and disabled file tree. Looked at commits and revisions; verified they reflected the setting correctly.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4813
Summary: This is the most requested feature in FB by far.
Test Plan:
Toggled, verified that data are saved.
Reloaded, toggled, toggled, toggled, verified that data are saved.
Reloaded.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4812
Summary: Applied fixes for issues mentioned in D4737#1&2
Test Plan: Verified that scripts seem to work as intended.
Reviewers: epriestley
CC: aran, Korvin
Maniphest Tasks: T2446
Differential Revision: https://secure.phabricator.com/D4782
Summary:
Try to lock the screen to whatever the user is looking at as we load changesets.
Notably, this improves the use case of taking a known action on a diff. Currently, you have to wait for everything to load or the comments keep getting scrolled down. After this change, the comments stay in the same place on screen.
Test Plan:
Raised the autoload changeset limit from 100 to 1000, looked at a 220 changeset diff.
- Reloaded it while scrolled at the top; normal behavior (no scrolling).
- Reloaded it, scrolled to the bottom. Comment area now stable.
- Reloaded it, kind of scrolled around the middle? Behavior seemed stable/reasonable. This one is kind of heursitic so it's hard to say I'm getting it totally right or not, but it's less important than the "bottom" case.
Reviewers: vrana, btrahan, chad, dctrwatson
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D4774
Summary: I cheated in a couple of places here, but this is in the process of getting refactored anyway, and there's a pretty clear boundary.
Test Plan: Viewed changesets in Differential, viewed standalone. Viewed context elements.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4723
Summary:
This accomplishes three major goals:
# Fixes phutil_render_tag -> phutil_tag callsites in DarkConsole.
# Moves the Ajax request log to a new panel on the left. This panel (and the tabs panel) get scrollbars when they get large, instead of making the page constantly scroll down.
# Loads the panel content over ajax, instead of dumping it into the page body / ajax response body. I've been planning to do this for about 3 years, which is why the plugins are architected the way they are. This should make debugging easier by making response bodies not be 50%+ darkconsole stuff.
Additionally, load the plugins dynamically (the old method predates library maps and PhutilSymbolLoader).
Test Plan:
{F30675}
- Switched between requests and tabs, reloaded page, saw same tab.
- Used "analyze queries", "profile page", triggered errors.
- Verified page does not load anything by default if dark console is closed with Charles.
- Generally banged on it a bit.
Reviewers: vrana, btrahan, chad
Reviewed By: vrana
CC: aran
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4692
Summary:
I broke this a bit a few revisions ago by making `phabricator_render_csrf_magic()` return array instead of string without actually grepping for callsites.
Instead of building a form in JS, build it in PHP and just change its action in JS. This is simpler and doesn't require us to expose CSRF magic in more places.
This change triggered a rather subtle bug: if we rendered a normal form on the page (and thus installed the "disable forms when submitted" behavior), the download button would incorrectly disable after being clicked. This doesn't currently happen in Files because we don't put a form on the same page as the download button.
Instead, make the "disable" behavior check if the form is downloading stuff, and not disable stuff if it is. Then mark the lightbox and Files form as both download forms.
Test Plan: Used lightbox; used Files. Verified download buttons download the right stuff and behave correctly. Grepped for other download forms, but all the other places where we write "download" don't appear to actually be download links.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4685
Summary: Notification menu shows a loading message before the menu is populated with the actual response.
Test Plan: Checked by having the function sleep between getting the response and populating the menu.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4696
Summary:
This is straightforward, except that `form.submit()` does not call onsubmit handlers. Create a `didSyntheticSubmit` event and have everything which listens for form submits listen for it too.
Fixes T704.
Test Plan: Hit control + enter in inline comments, main commetns, Pholio, conpherence. Verified it triggered appropriate JS (workflow / special behaviors).
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T704
Differential Revision: https://secure.phabricator.com/D4704
Summary: Mock page now shows all images. Switching between images is done by clicking thumbnails.
Test Plan: Verified that all images are shown. Verified that by clicking thumbnail the image clicked will show.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, AnhNhan
Maniphest Tasks: T2427
Differential Revision: https://secure.phabricator.com/D4698
Summary: decent title. Stylistically its probably a bit rough. Also, I think @chad describes an even hotter workflow in T2418. Note this removes the "default image" thing which I don't think makes sense conceptually since by default the image changes to who replied last...
Test Plan: uploaded an image - worked. uploaded a txt file - got ugly errors that file was not supported.
Reviewers: epriestley, chad
Reviewed By: chad
CC: aran, Korvin
Maniphest Tasks: T2417
Differential Revision: https://secure.phabricator.com/D4668
Summary: it's ugly. but it works. basically. See T2399 for a roughly prioritized list of what still needs to happen.
Test Plan:
- created a conpherence with myself from my profile
- created a conpherence with myself from "new conpherence"
- created a conphernece with another from "new conpherence"
- created a conpherence with several others
- created a conpherence with files in the initial post
- verified files via comment text ("{F232} is awesome!") and via traditional attach
- edited a conpherence image
- verified it showed up in the header and in the conpherence menu on the left
- edited a conpherence title
- verified it showed up in the header and in the conpherence menu on the right
- verified each widget showed up when clicked and displayed the proper data
- calendar being an exception since it sucks so hard right now.
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: aran, epriestley, chad, codeblock, Korvin
Maniphest Tasks: T2301
Differential Revision: https://secure.phabricator.com/D4620
Summary: This needs some tweaks but I'll follow up with @DeedyDas in T2353.
Test Plan: So many memes.
Reviewers: chad, btrahan, DeedyDas
Reviewed By: chad
CC: aran
Maniphest Tasks: T2353
Differential Revision: https://secure.phabricator.com/D4616
Summary:
Submoduling is slightly convenient for developers but hellishly difficult for many users. Since we make about a dozen updates to Javelin per year, just include the source directly.
Even if we run `git submodule status` more often, this creates additional problems for users with PATH misconfigured.
Fixes T2062 by nuking it from orbit.
Test Plan: Loaded site, browsed around. Grepped for references to submodules.
Reviewers: btrahan, vrana
CC: aran
Maniphest Tasks: T2062
Differential Revision: https://secure.phabricator.com/D4581
Summary: Removes the panel-view on login and adds additonal responsive styles for mobile forms.
Test Plan: View in mobile browser, resize page.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4530
Summary: They're buggy and I'm not going to get to fixing them for a bit and they trigger on Macbook Airs and such.
Test Plan: Reloaded a revision in a narrow window.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D4490
Summary:
See discussion in D4438. Allows users to customize application tiles, and implements generally reasonable defaults so they hopefully won't.
Sizes are "invisible" (internal only, used to hide admin apps from non-admins), "hidden" (hide by default, show after clicking "Show More Applications"), "show" (show a small square tile) and "full" (show a full-width tile with subtitle).
Test Plan:
Default view for a non-admin:
{F29375}
Adjusted settings, hidden:
{F29373}
Adjusted settings, shown:
{F29374}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D4439
Summary: We need to nuke `has-drag-nav` too when the user presses "F" in Differential/Diffusion.
Test Plan: Pressed "F" in Differential.
Reviewers: zeeg, chad, btrahan
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D4457
Summary:
- Add a query parameter to select the diff renderer.
- When we populate diffs, use the 1-up renderer if the device isn't a desktop. Don't switch renderers once the page has loaded since it will be a tremendous mess. We can add an "always use 1-up" preference later.
- Tweak JX.Device so we avoid a race condition, where `JX.Device.getDevice()` may not be valid when other behaviors run.
- Implement enough of the 1-up HTML renderer to provide a vague hint that it actually works.
- Fix a couple of bugs with primitive construction.
Test Plan:
{F29168}
{F29169}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D4423
Summary:
Fixes T181. I actually have no idea what the original issue in T181 was, but this fixes several problems:
- The code for figuring out whitespace-only changes was extremely confusing and probably buggy (the code for figuring out unchanged files was equally confusing but probably less buggy).
- When rendering a whitespace shield, we did not offer a "Show Changes" link. Instead, show the "Show Changes" link: we can always render the content beneath this link.
- When clicking "Show Changes", we used the current whitespace mode, which might result in a diff with no changes. Instead, force "show all" whitespace mode.
- We never offered a "show changes" link for unchanged files. Instead, offer this link if we can service it.
- When clicking "Show Changes", we pierced the shield but didn't force file content, which would fold the entire file even if it was available. Instead, force the file content.
Test Plan: Generated whitespace-only and no-change diffs. Clicked shield links, or verified no shield link available in no-change-with-no-content. Generated an "@generated" change, verified shield worked correctly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T181, T2009
Differential Revision: https://secure.phabricator.com/D4407
Test Plan: Clicked on next month, didn't see year 20121.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4277
Summary:
When previewing, save drafts. When loading objects, restore drafts if they are available.
Depends on: D665
Test Plan:
- Viewed a Mock.
- Typed text into the comment box.
- Reloaded the page.
- Text still there.
- Hit submit, got my comment.
- Reloaded the page.
- Draft correctly deleted.
- Repeated for Macros.
Reviewers: btrahan, chad, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2104
Differential Revision: https://secure.phabricator.com/D4252
Summary:
Implements previews for Macros and Pholio.
(Design is nonfinal -- kind of split the difference between `diff_full_view.png`, laziness, and space concerns. Next couple diffs will add more stuff here.)
Test Plan: {F28055}
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran, vrana
Maniphest Tasks: T2104
Differential Revision: https://secure.phabricator.com/D4246
Summary: 'cuz new fluid layouts require the westerlyness. Looks like D4126 started the N and W implementation but didn't finish it...? note I had to do the shifting of the 5 pixels in javascript; using the CSS didn't work for me in chrome.
Test Plan: uiexample, and hoping it goes well when deployed in prod for differential case
Reviewers: epriestley
Reviewed By: epriestley
CC: chad, aran, Korvin
Maniphest Tasks: T2211
Differential Revision: https://secure.phabricator.com/D4257
Summary: We now remember this value, it's remembered also after page refresh.
Test Plan: Reloaded revision about to be accepted.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4196
Summary:
Modernizes file uploads. In particular:
- Adds a mobile menu, with an "Upload File" item.
- Adds crumbs to the list view, detail view and upload view.
- Adds "Upload File" action to crumbs.
- Moves upload file to a separate page.
- Removes the combined upload file + recent files page.
- Makes upload file use a normal file control by default (works on mobile).
- Home page, file list and file upload page are now global drop targets which accept files dropped anywhere on them. Dragging a file into the window shows a mask and an instructional message.
- User education on this is a little weak but I think that's a big can of worms?
- Fixes a bug where dropping multiple files into a Remarkup text area produced bad results (resolves T2190).
T879 is related, although it's specifically about Maniphest. I've declined to make global drop targets yet there because there are multiple drop targets on the page with different meanings. That UI needs updating in general.
@chad, do we have an "upload" icon (counterpart to "download")?
Test Plan: Uploaded files in Maniphest, Differential, Files, and from Home. Dragged and dropped multiple files into Differential. Used crumbs, mobile.
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2190
Differential Revision: https://secure.phabricator.com/D4200
Summary: Update to new UI stuff, prepare for mobile.
Test Plan: {F27167}
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D4198
Summary:
- Fixes a bug where dragging a flexible left nav and then pressing "f" to hide it didn't properly reset the content panel's left margin.
- Fixes a bug where D4185 removed "white-space: nowrap;" accidentally (from @asherkin)
Test Plan:
- Dragged bar, then pressed "f".
- Can't repro the nowrap thing for some reason.
iiam
Reviewers: asherkin, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4194
Summary:
Fixes glitches in the side nav. Resolves T1828. Resolves T2156.
- Elastic scrolling (T2156): in Safari on OSX, using a scroll touch on the trackpad to scroll up past the top of the document caused newer-style side menus to scroll down, leaving a visible whitespace bar.
- Whitespace glitch: Particularly in Safari, scrolling down the document quickly from the top caused the top menu to scroll away before the side menu rose to meet it. Use a fixed background color bar that extends under the menu so this doesn't happen.
- Use of "!important": use CSS better so we don't need to "!important" things.
- Dark Console (T1828): Instead of hard-coding the top position, determine it dynamically by looking at where the content is. This also fixes the menu overlapping with the red "there are errors on this page" development bar.
- General "fixed" glitchiness: don't use fixed-position for menu content other than flexible (draggable) menus.
Test Plan:
- Viewed and scrolled menus in Paste. Opened and closed DarkConsole. Switched devices.
- Viewed and scrolled flexible menus in Differential and Diffusion. Opened and closed DarkConsole. Switched devices.
Reviewers: vrana, chad, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1828, T2156
Differential Revision: https://secure.phabricator.com/D4185
Summary:
We don't own the copyright to (or have a license to distribute) this media file.
Also improve performance.
Test Plan: ↑↑↓↓←→←→BA <Enter>
Reviewers: codeblock
Reviewed By: codeblock
CC: aran
Differential Revision: https://secure.phabricator.com/D4176
Summary:
When possible, render application transactions via Ajax. Instead of reloading the page when the response returns, append new transactions to the transaction view.
Scroll the window to the new transactions, animate them in, and clear the form to make this interaction feel reasonable.
When editing transactions, fade them in but do not scroll to them (i.e., don't disrupt the user's position).
Test Plan: Edited and appended transactions via Ajax. Observed fade in animations and scroll behavior. Clicked anchors to verify proper anchor accounting.
Reviewers: vrana, btrahan, chad
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1960
Differential Revision: https://secure.phabricator.com/D4151
Summary: When possible, replace the edited or deleted transaction inline using Ajax.
Test Plan: Repeatedly edited and deleted transactions. Clicked their anchors to verify anchors were correctly preserved.
Reviewers: vrana, btrahan, chad
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1082
Differential Revision: https://secure.phabricator.com/D4150
Summary:
Allows you to edit or delete comments in appplications which support ApplicationTransactions.
UI/UX stuff:
- The dialogs are rough but I want to do a dialog design pass more generally, @chad has some mocks.
- When you add new mentions via edit, they don't currently count as mentions. I'm not sure what I want to do about this.
- When you edit or delete a comment, we do not publish any notifications about it. I think this is reasonable.
- I didn't separate "delete" out versus "edit"; I assume it will be reasonably intuitive that deleting all the text deletes effectively deletes the comment. I also want to discourage deletion somewhat (we still show the transaction, just show that the comment has been deleted).
Test Plan:
Transaction view, note "Edit" and "Edited" links:
{F26914}
Edit view, has some design issues but I want to do a pass on dialogs in general:
{F26915}
History view:
{F26913}
Reviewers: vrana, btrahan, chad
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1082
Differential Revision: https://secure.phabricator.com/D4149
Summary:
- Adds mail support to the generic transaction construct.
- Restores mail support to Pholio (now much improved; the mails are actually useful).
Test Plan: Updated a Pholio mock, got mail.
Reviewers: btrahan, chad, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2104
Differential Revision: https://secure.phabricator.com/D4139
Summary:
- Gets about 25% of the way toward @chad's notification mocks.
- YES: Hover states, entire notification is a click target, border, header, footer.
- NO: Profile pictures (lazy), timestamps (want to refactor time code before introducing a new formatting style), app icons (they'd look funny without timestamps I think)
- Deletes some old files.
- Mostly trying to get this good enough to turn on by default.
Test Plan: Looked at notifications. Clicked some notifications.
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D4119
Summary: Add a ".device" rule which means "phone or tablet". Simplify about 5000 rules which were written ".device-phone X, device-tablet X { ... }".
Test Plan: Browsed the site a bit without incident.
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1960
Differential Revision: https://secure.phabricator.com/D4103
Summary:
Add a basic breadcrumbs element, and implement it in Paste.
This needs some polish but is most of the way there.
Test Plan:
{F26443}
{F26444}
{F26445}
(This element is not visible on devices.)
Reviewers: chad
Reviewed By: chad
CC: aran, btrahan
Maniphest Tasks: T1960
Differential Revision: https://secure.phabricator.com/D4087
Summary:
Toss this completely as per discussion elsewhere. Basically it doesn't feel as useful as we imagined it would, and breadcrumbs from T1960 will replace the primary useful part (navigating up).
There's some more cleanup to do but I'll hit that in the next few diffs.
Closes T1828 as wontfix.
Test Plan: Viewed app + local, app-without-local interfaces. Saw no app menus.
Reviewers: chad
Reviewed By: chad
CC: aran, vrana
Maniphest Tasks: T1828, T1960
Differential Revision: https://secure.phabricator.com/D4033
Summary: Switch to the final versions of these
Test Plan: Will add screenshots...
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D4032
Summary:
When users middle click or command-click an image, we should open it in a new tab, not open a lightbox.
See https://github.com/facebook/phabricator/issues/234
Test Plan: Left, middle, and command-clicked a lightbox image.
Reviewers: vrana, chad, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4020
Summary:
In Differential, viewer can hit 'f' key to hide/show file tree on the left
side. Useful on narrow monitors.
Test Plan: Open any diff in Differential tool, hit 'f', watch file tree disappears
Reviewers: vrana, mattchoi, epriestley
Reviewed By: vrana
CC: aran, epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D3844
Summary:
Clicking on a shielded file in file tree highlighted the previous one.
Also, menu bar is not fixed anymore.
Test Plan: D3355#d77999bc - `scripts/celerity_mapper.php` was highlighted.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3806
Summary:
Minor tweaks to lightboxes.
- With "position: fixed;", we don't need to do any of the scroll/resize stuff. Just remove it.
- Make the lightbox go over the menu bar -- was it intentional that it wasn't?
- Make 'jx-mask' use "position: fixed;" too.
- Add a loading indicator.
- In Differential/Maniphest/etc, a preview may bring in an image but won't bring in the CSS we need. The "real" fix is to ship CSS/JS with ajax, but that's really hard -- fake it by pulling in the right CSS any time we render a remarkup area.
I'm going to do a couple of other tweaks here but need to update JX.Mask.
Test Plan: Verified behavior is reasonable in Safari, Firefox, Chrome with multiple images / scroll / previews / resize.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1896
Differential Revision: https://secure.phabricator.com/D3795
Summary: We can let the browser do the scaling with some simpler CSS rules.
Test Plan: Opened very large images in Safari, Firefox and Chrome and resized the browser. Observed smooth scaling and no issues with the image overlapping UI elements, etc.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1896
Differential Revision: https://secure.phabricator.com/D3802
Summary: When you click the dark background, close the lightbox.
Test Plan: Clicked arrows, image, etc., to make sure it didn't close. Clicked background to close.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3801
Summary: Our "html { overflow-y: scroll; }" makes Safari flip out when we put "hidden" on body. Instead, put the scroll on `body` and then replace it with `hidden` when the lightbox is visible.
Test Plan: In Safari, the body scrollbar vanishes when the lightbox is active and scrolling no longer causes spasms.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3800
Summary:
See discussion in T404. Basically, the problem with date-only controls is that they may behave unpredictably in the presence of timezones. When you say "This needs to be done by Oct 23", you probably mean "Oct 23 5PM PST" or something like that, but someone in China may see the "Oct 24" and hit the deadline in good faith but be 10 hours too late. T404 has more discussion and examples. There are ways to fake this, but they get more complicated if the guy in China needs to move the date forward 24 hours.
I think the best solution to this is to not have date-only controls, and always display the time. This makes it absolutley unambiguous what something means, because the guy in the US will set "Oct 23 5PM" and the guy in China will see that accurately in local time.
The downside is that it's slightly more visual clutter and work for the user to specify things precisely, but I added some hints (start/end of day, start/end of business) that will hopefully let us pick the right default in most cases.
Test Plan:
Set some dates.
{F21956}
This has a couple of edge case issues on resize and some not-so-edge-case issues on mobile, but should be good to build T407 on without API changes.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T404, T407
Differential Revision: https://secure.phabricator.com/D3793
