Summary:
We have some complaints on this feature:
- It's not clear that the displayed code is context from gap.
- It would be better if the context would be displayed with its real indentation.
- It's not clear how far the context is from the displayed code.
- Links revealing gap aren't on consistent place.
This solves all these problems and introduces new one:
It now seems that the reveal links works only with the left side.
Anyway, I think that this is better overall.
I don't want to put the context on a separate line to not waste space.
Test Plan: Displayed various contexts, revealed context.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, bh, jwatzman
Differential Revision: https://secure.phabricator.com/D3404
Summary:
Many images in Differential changesets are icons designed for
use on dark backgrounds. This makes them invisible on Differential's
white background. This adds an option to use a darker background
instead so you can see the images.
Currently this behavior is triggered on hover. (Also, it's a rather
garish fuchsia.) It seems fine UX-wise but I'm not totally sure of it.
Test Plan:
Load diff containing grippy_texture. Marvel at the grippy
fuchsia.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3272
Summary:
Diff of diffs display changes between new versions of file.
This is bad after rebase because there can be many unrelated changes so it is hard to spot the real change.
This diff unhighlights the lines that were added or removed in rebase.
The changes are still visible (they can be sometimes relevant) but very subtle.
Test Plan:
# Add, change and delete line. Display diff.
# Add and change some lines in parent. Rebase. Display diff. Display diff of diff.
# Change and add some lines. Display diff. Display diff to first diff. Display diff to second diff.
Reviewers: epriestley, jungejason
Reviewed By: jungejason
CC: jungejason, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2761
Summary:
The highlighting is distracting according to Nick Shrock and others.
Real designer, Lee Byron, helped me with this.
It also gives us unagressive target for jumping to the source line in future.
Another feature I will probably implement is highlighting also the source of copies/moves.
I will use the right side of the left column for it.
Test Plan:
Hover copied notifier.
Hover coverage notifier.
I've also checked that this doesn't break our super-flaky old/new code JavaScript detector.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin, leebyron, schrockn
Differential Revision: https://secure.phabricator.com/D2443
Summary:
The highlighting is distracting according to Nick Shrock and others.
Real designer, Lee Byron, helped me with this.
It also gives us unagressive target for jumping to the source line in future.
Another feature I will probably implement is highlighting also the source of copies/moves.
I will use the right side of the left column for it.
Test Plan:
Hover copied notifier.
Hover coverage notifier.
I've also checked that this doesn't break our super-flaky old/new code JavaScript detector.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin, leebyron, schrockn
Differential Revision: https://secure.phabricator.com/D2403
Summary: This allows writing inline comments and reduces different behavior between normal and very large diffs.
Test Plan:
Verify that normal diff works.
Verify that very large diff works.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2361
Summary:
Also reduce the memory usage a little bit (before increasing it again).
I use the same CSS class as for the copied code.
Test Plan: Parsed 100 diffs and checked about 10 of them - looks good.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2339
Summary: The color used for this feature is pretty important and I am bad with colors.
Test Plan:
View diff created by D2320 with some copied lines and one line changed:
{F10604, size=full}
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2321
Summary:
- For line numbers, use "user-select: none" to make them unselectable. This provides a stronger visual cue that copy/paste is enchanted.
- In Paste, make it look sensible again after the blame-on-blame refactor in Diffusion. See also TODO to share this code formally.
- In Diffusion, use the "phabricator-oncopy" behavior.
NOTE: I left blame/commit columns selectable in Diffusion, since you might reasonably want to copy/paste them?
NOTE: In Differential, the left side of the diff still highlights, even though it will be copied only if you select part of a line on the left and nothing else. But this seemed like a reasonable behavior, so I left it.
Test Plan:
- Looked at Paste. Saw a nice line number column. Selected text, got the expected selection. Copied text, got the expected copy.
- Looked at Diffusion. Saw a nice line number column, still. Selected text, got expected selection. Copied text, got expected copy.
- Looked at Differential. Highlighted stuff, got expected results. Copied stuff, got expected results.
Reviewers: btrahan, vrana, jungejason
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1123
Differential Revision: https://secure.phabricator.com/D2242
Summary: Partially broken by D2166.
Test Plan:
Hover line number in revision.
Hover line number in standalone view.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2196
Summary: See T955. We jump to an awkard place right now; jump above the comment instead.
Test Plan: Clicked inline comment anchor links.
Reviewers: Makinde, btrahan
Reviewed By: Makinde
CC: aran, epriestley
Maniphest Tasks: T955
Differential Revision: https://secure.phabricator.com/D2029
Summary:
It looks like there is really this text written e.g. at https://secure.phabricator.com/D1896#0a6a1957
I am not sure that it is the only place which needs to be fixed.
Test Plan: Display diff with no newline at end of file in Differential.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1900
Summary:
- Show a tip in the margin about what coverage colors mean.
- Highlight the line when mousing over coverage.
- Randomly change the colors to different colors.
- Fix a bug with "show more" that I introduced with the other coverage diff (oops!)
Test Plan: Moused over coverage things.
Reviewers: tuomaspelkonen, btrahan
Reviewed By: tuomaspelkonen
CC: aran, epriestley
Maniphest Tasks: T965
Differential Revision: https://secure.phabricator.com/D1874
Summary:
The current approach of using a modal overlay dialog to create/edit inline
comments is pretty silly. Use an inline textarea instead.
This element isn't perfect and we have some mild modalness issues, but I think
it's better than the silly thing we've got going on right now. We can keep
poking it as people break it.
Test Plan:
- Created comments; submitted and undid them in empty and nonempty states.
Used undo for nonempty states + cancel.
- Edited comments; saved and canceled them. Used undo for changed state.
- Replied to comments; yada yada as above.
- Deleted comments.
- Did various modal trickery where I clicked "Reply" on something else with a
dialog already up, this very mildly glitches but I think it's not a big issue.
Reviewers: vrana, btrahan, Makinde, nh
Reviewed By: vrana
CC: aran, epriestley
Maniphest Tasks: T431
Differential Revision: https://secure.phabricator.com/D1716
Summary:
Render coverage information in the right gutter, if available.
We could render some kind of summary report deal too but this seems like a good
start.
Test Plan:
- Looked at diffs with coverage.
- Looked at diffs without coverage.
- Used inline comments, diff-of-diff, "show more", "show entire file", "show
generated file", "undo". Nothing seemed disrupted by the addition of a 5th
column.
Reviewers: btrahan, tuomaspelkonen, jungejason
Reviewed By: btrahan
CC: zeeg, aran, epriestley
Maniphest Tasks: T140
Differential Revision: https://secure.phabricator.com/D1527
Summary: See T551. We don't apply the default monospacing rules to the example,
so if you don't have a custom font selection you don't see the default
accurately.
Test Plan: Deleted my preference, saw an accurate default. Set my preference to
"14px impact", ensured it was respected in applications.
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: nh
CC: aran, nh
Differential Revision: 1035
Summary:
When we highlight specific changes (use the '.bright' css class), we override syntax highlighting with 'color:'.
This commit makes us stop doing that, by removing the 'color:'.
Test Plan:
My local instance sucks, so I can't test this :P @epriestley? :P
Reviewers:
epriestley
CC:
Differential Revision: 778
Summary:
Permit "j" and "k" to cycle through individual changeblocks, similar to how this
feature works in ReviewBoard. This still needs a bunch of refinement but it's
getting closer to being useful.
Also moved reticle underneath the table so you can click links through it (derp
derp).
Test Plan:
Used "j" and "k" to cycle through individual changes.
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen
CC: moskov, aran, epriestley
Differential Revision: 426
Summary:
See attached tasks. See D459 for the ability to merge tasks.
Test Plan:
Looked at posted and unposted inline comments.
Reviewed By: aran
Reviewers: edward, viyer, aran, jungejason, tuomaspelkonen
CC: aran, epriestley
Differential Revision: 461
Summary:
When a user hits 'cancel' on a 'new', 'edit', or 'reply' operation, add a little
"Changes discarded. __Undo__" insert so they can get their change back. No undo
for delete since there's an explicit prompt. Once this lands we can make
'escape' work again to close dialogs.
This change started feeling really good when I was merging all the duplicate
code and making things more consistent, but by the time I started writing client
rendering it felt gross. I'm not really thrilled with it but I guess it's a step
forward? The feature seems pretty OK in practice. Let me know how much barfing
this causes and I can try to remedy the most acute concerns.
This also fixes a bug where replies always (?) appear on the 'new' side of the
diff (I think?).
Test Plan:
Applied 'new', 'edit', 'delete' and 'reply' operations, pressed 'cancel' and
'okay' in each case, with and without changing text where relevant. All
behaviors seem to conform with expectations, except that canceling out of 'edit'
without changing the text gives you an option to undo when it shouldn't really.
There's no super easy way to get at the original text right now.
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen
CC: simpkins, aran, epriestley
Differential Revision: 406
Summary:
Never converted this TODO over from XHP.
NOTE: This looks terrible since the CSS didn't make it over, can one of you grab
the rules for .differential-property-table and .property-table-header? If they
aren't still in trunk, try history for html/intern/css/tools/differential/
Test Plan:
Made a property change, looked at it in Differentila.
Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran
CC: codeblock, aran
Differential Revision: 409
Summary:
Add links to the 'standalone view' to grab the raw source text. I think this
operation is rare enough that it's okay to hide it like this. I changed
'Standalone View' to 'View Standalone / Raw' to improve discoverability.
This also fixes the broken Standalone View links in Diffusion by no longer
rendering them.
Test Plan:
viewed old and new sources for a changeset
Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen, jungejason, aran
CC: aran, tuomaspelkonen
Differential Revision: 243
Summary:
- Prevent long comments from expanding the inline box.
- Make anchor links to inlines work properly.
- Get rid of "pre" white-space formatting in inline comments.
Test Plan: Viewed a revision with crazy comments in it.
Reviewers:
CC:
