Paths

Table of Contentst

Differential D15487

Update Diffusion UI
ClosedPublic
Actions

Authored by chad on Mar 16 2016, 10:48 PM.

Details

Reviewers

epriestley

Commits

Restricted Diffusion Commit
rP8f94aa8a0647: Update Diffusion UI

Summary

This updates (all?) of Diffusion/Audit to new UI, included edit and other extra form pages. It's fairly complete but I don't know all the nooks and crannies so to speak to fully verify I didn't mess anything up.

Test Plan

Tested creating new repositories, browsing, searching, auditing. Need more eyes.

Diff Detail

Repository

rP Phabricator

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

chad updated this revision to Diff 37324.Mar 16 2016, 10:48 PM

chad retitled this revision from to Update Diffusion UI.

chad updated this object.

chad edited the test plan for this revision. (Show Details)

chad added a reviewer: epriestley.

Herald added a subscriber: Korvin. · View Herald TranscriptMar 16 2016, 10:48 PM

Looks great to me overall. I was expecting to be a little hesitant on some of these interfaces but I pretty much really like everything.

I couldn't find many real issues, mostly nitpicky stuff:

Technical stuff:

I don't think this is related, but the "Table of Contents" on READMEs is missing some margin/padding:

Screen Shot 2016-03-17 at 6.41.12 AM.png (458×902 px, 52 KB)

The "Tags" table on the main repository homepage doesn't have the blue header style that the other tables do:

Screen Shot 2016-03-17 at 6.42.58 AM.png (378×513 px, 44 KB)

The "All Branches" and "All Tags" detail views have inconsistent table wrapping (one's a bare table, one has a wrapper header?):

Screen Shot 2016-03-17 at 6.43.50 AM.png (184×349 px, 13 KB)

Screen Shot 2016-03-17 at 6.43.45 AM.png (226×384 px, 16 KB)

Double bottom border on commit detail pages at the end of the changesets? Probably not related.

Screen Shot 2016-03-17 at 6.54.35 AM.png (188×378 px, 18 KB)

There are two top-secret "Lint" interfaces which are nearly impossible to get to and which have odd padding/margin issues. There's virtually no chance you can access these locally, but I can counterdiff you once this lands. They're usable, just missing some margin/blue:

Screen Shot 2016-03-17 at 7.02.27 AM.png (998×1 px, 206 KB)

Screen Shot 2016-03-17 at 7.02.44 AM.png (832×1 px, 212 KB)

On search results page, missing (?) box around controls? Might be intentional since it looks OK, just seemed unusual to me:

Screen Shot 2016-03-17 at 7.06.08 AM.png (470×1 px, 61 KB)

Product stuff / general feedback:

The clone stuff on home jumps out at me more as ugly since the rest of the page looks better, but that's on me -- I'm hoping we can fix it after T10366.
Browse feels a tiny bit weird but there are some options we could add there now that might help flesh it out (e.g., sort directories first). Maybe we should shorten the date columns, too (e.g., only show "May 13" for same year, "9:12 AM" for same day)? They feel kind of cluttered now with the simpler table.
Full-width README on "Browse" feels a little weird, maybe? It's consistent with home but there's no full-width content here.

Screen Shot 2016-03-17 at 6.48.40 AM.png (915×1 px, 175 KB)

I like the "Show Search" compromise on Browse. I think this is an interface we can improve in the future, but I think this is a good step to balance concerns for now.
On commits, in the header, maybe use $commit->getLocalName()? That will produce aabbccdd instead of rXYZaabbccdd. I don't think we need the extra prefix here, and as long as we have that element we might as well make it easy to copy/paste?

Screen Shot 2016-03-17 at 6.52.30 AM.png (90×322 px, 12 KB)

Maybe omit policy header tag for file browsing? It's always the same as the repository, and we don't show it on other pages like "Commit". I don't think it's useful on any sub-pages.

Screen Shot 2016-03-17 at 6.56.35 AM.png (153×457 px, 21 KB)

On file browse, we should probably turn these into actions at some point.

Screen Shot 2016-03-17 at 6.56.28 AM.png (222×463 px, 14 KB)

I was a little worried about making file browse not full-width but it actually seems fine in practice.
Commit detail can definitely be refined but the biggest issue (parsing messages) is on me and I don't think it got worse or anything, the unparsed messages were just sketchy to start with.
I think you adjusted these at some point, but the black folder icons feel a little bit intense to me with the bluer color scheme:

src/applications/diffusion/controller/DiffusionController.php
306–315	Odd indent.
315	Oh, I think this is the thing I suggest replacing with `getLocalName()`. Since you're using `formatCommitName()`, the fix is to add a second `true` parameter for the "local" format: `formatCommitName($stable_commit, true)`.

This revision is now accepted and ready to land.Mar 17 2016, 2:14 PM

I converted "Tags" and "Symbols" blind, any short path to testing those locally? I probably should build symbols locally, but dont know what Tags is about.

If you import libphutil locally it should have a couple of tags you can test, that's probably the easiest thing.

Otherwise, if you have a test repo, you can use git tag to tag stuff.

Let me look at symbols, I didn't check that.

Symbols looks fine to me locally:

Screen Shot 2016-03-17 at 7.23.27 AM.png (1×1 px, 141 KB)

Re-write PropertyListView on DiffusionCommit

On commits, in the header, maybe use $commit->getLocalName()? That will produce aabbccdd instead of rXYZaabbccdd. I don't think we need the extra prefix here, and as long as we have that element we might as well make it easy to copy/paste?

Any hints here, I can't seem to easily alter this function.