Page MenuHomePhabricator

Convert Differential to new layout
ClosedPublic

Authored by chad on Mar 11 2016, 4:54 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Apr 25, 1:58 AM
Unknown Object (File)
Wed, Apr 24, 10:36 AM
Unknown Object (File)
Sun, Apr 21, 5:15 AM
Unknown Object (File)
Wed, Apr 3, 9:42 AM
Unknown Object (File)
Wed, Apr 3, 8:21 AM
Unknown Object (File)
Tue, Apr 2, 4:42 PM
Unknown Object (File)
Mon, Apr 1, 3:19 PM
Unknown Object (File)
Mar 9 2024, 2:43 PM
Tokens
"Piece of Eight" token, awarded by epriestley.

Details

Reviewers
epriestley
Commits
Restricted Diffusion Commit
rP148a50e48be1: Convert Differential to new layout
Summary

First pass at converting Differential, I likely have some buggy-poos but thought I'd toss this up now in case very bad bugs present.

To do:

  • Need to put status back on Hovercards
  • "Diff Detail" probably needs a better design
Test Plan

Looking at lots of diffs, admittedly I dont have harbormaster, etc, running locally. Checked Diffusion for Table of Content changes on small and large commits.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

chad retitled this revision from to Convert Differential to new layout.
chad updated this object.
chad edited the test plan for this revision. (Show Details)
chad added a reviewer: epriestley.
chad edited the test plan for this revision. (Show Details)

Couple of technical things. Some of this is probably hard to reproduce locally since you need a bunch of Drydock/Harbormaster config or other weird custom stuff to activate some of the sections. I can likely tweak anything you can't easily sort out -- I think it's all pretty simple, just hard to get the elements to show up.

  • I think we should leave active landing operations on top, they seem pretty important? This element should only be visible when it's highly relevant (active operation ongoing, or failure) so I think it's OK to let it stay up top?

Screen Shot 2016-03-12 at 7.31.17 AM.png (489×1 px, 54 KB)

  • "Related changes affecting these files" has lost a header/box:

Screen Shot 2016-03-12 at 7.32.22 AM.png (216×1 px, 34 KB)

  • "Unit Tests" table is getting some weird header stuff since it's still and old-style box:

Screen Shot 2016-03-12 at 7.34.21 AM.png (703×1 px, 215 KB)

  • Custom revision header warnings no longer work. Here's master (warning provided by a custom field):

Screen Shot 2016-03-12 at 7.39.01 AM.png (268×659 px, 27 KB)

...but it's gone in this patch:

Screen Shot 2016-03-12 at 7.39.15 AM.png (391×511 px, 29 KB)

  • I'm getting this stack trace locally, not sure what the root cause is. I can chase it down:

Screen Shot 2016-03-12 at 7.41.50 AM.png (505×1 px, 242 KB)

  • These elements looks a little odd, particularly with the margins (probably not new):

Screen Shot 2016-03-12 at 7.43.37 AM.png (178×1 px, 29 KB)

Screen Shot 2016-03-12 at 7.48.32 AM.png (242×1 px, 50 KB)

  • The "Z" key ("haunted comments") isn't working for me.

Product/design stuff:

  • This feels good to me overall, doesn't seem weird or anything. Like Diffusion, I think we can probably refine some stuff, but big picture feels totally reasonable.
  • Author in sidebar is maybe weird on revisions, which are strongly-authored to me, but this is a general question we have a little bit of product tension on across all the new stuff. I don't think we need to resolve it immediately.
chad planned changes to this revision.Mar 12 2016, 4:15 PM

i'll get you a new diff this afternoon.

  • Landing operations now on top
  • Legalpad Warning now correctly spaced
  • Moved author to Subheader
  • Remove 'noBox' from Recent Similar Revisions
  • Unit Test have better headers
  • Revision warnings should appear
chad edited the test plan for this revision. (Show Details)
chad edited edge metadata.
  • fix comments

not sure about haunt mode or that error_log.

  • remove wrap_id? can't find what this does
In D15463#178276, @chad wrote:
  • remove wrap_id? can't find what this does

Blame indicates it went with arc_project which doesn't exist anymore.

This is ready for re-review

I'll figure out what's up with that error log, one sec..

epriestley edited edge metadata.

Revision list got a little weird, not intended? CSS spilling out?

Screen Shot 2016-03-12 at 12.55.50 PM.png (292×403 px, 29 KB)

Looks good otherwise. Error log doesn't repro any more, so I think you fixed whatever it was (or: ghosts).

This revision is now accepted and ready to land.Mar 12 2016, 8:58 PM
chad edited edge metadata.
  • Fix background leakage
This revision was automatically updated to reflect the committed changes.