Page MenuHomePhabricator

2017 Week 20 Transactions Errata
Closed, ResolvedPublic

Assigned To
Authored By
epriestley
May 19 2017, 1:17 PM
Referenced Files
F4967319: Screen Shot 2017-05-19 at 11.26.10 AM.png
May 19 2017, 6:27 PM
F4967313: Screen Shot 2017-05-19 at 11.20.49 AM.png
May 19 2017, 6:23 PM
F4967309: Screen Shot 2017-05-19 at 11.07.22 AM.png
May 19 2017, 6:08 PM
F4967305: Screen Shot 2017-05-19 at 11.06.54 AM.png
May 19 2017, 6:08 PM
F4967281: Screen Shot 2017-05-19 at 10.47.37 AM.png
May 19 2017, 5:50 PM
F4967207: Screen Shot 2017-05-19 at 9.06.34 AM.png
May 19 2017, 5:50 PM
F4967188: Screen Shot 2017-05-19 at 8.57.00 AM.png
May 19 2017, 4:06 PM
F4967186: Screen Shot 2017-05-19 at 8.54.14 AM.png
May 19 2017, 4:06 PM

Description

(Placeholder for whatever I manage to dig up.)

Event Timeline

Changes this week are:

  • Bunch of changes to project UIs, mostly around workboard management.
  • Pholio
  • Inlines
  • Phriction
  • Instances
  • A big chunk of Projects transactions

I'm going to run through all this stuff and see if I can catch anything that we missed in review/testing.

Oh, yeah -- that technically promoted last week but I probably should really be looking at the last two weeks, since the previous roundup in T12685.

Oh, no it didn't. I just can't read.

This state on projects might be a little misleading: here, theproject supports subprojects/milestones but the viewer does not have permission to edit the project, so they can not actually create them. The text means "It is possible to create milestones for this project" but the phrasing implies the viewer, specifically, can:

Screen Shot 2017-05-19 at 8.54.14 AM.png (277×329 px, 21 KB)

"Archived" lacks an icon while "member" / "watching" have them, and we can get a 3-state stack now -- maybe remove "archived" and just let the grey UI speak for itself? Although this communicates information with only color.

Screen Shot 2017-05-19 at 8.57.00 AM.png (101×901 px, 17 KB)

Also might be nice to put an effect on profile images for disabled objects, similar to the one that disabled users now get.

Column history page just has an empty curtain? Plans here, or kick it to full width? Or just leave it idk

Screen Shot 2017-05-19 at 8.59.01 AM.png (868×1 px, 113 KB)

The item ordering in this menu could maybe be improved:

Screen Shot 2017-05-19 at 8.59.43 AM.png (234×248 px, 20 KB)

e.g. put column actions together without coloring in the middle, like:

+ Add Column
x Reorder Columns
o Show Hidden Columns
---
/ Change Background Color
---
* Manage Workboard

But this is pretty fluff, and there's an argument that "hidden / backgroud" are both "display" items and should actually be grouped.

We allow creation of columns with no name, and just name them "unnamed column". We need to support this to some degree for backlog, but it seems a little silly for normal columns.

Overall, board actions feel a lot more sensible to me now.

column history full width had other display bugs I didn't feel like writing CSS for.

These hashtag transactions should use a renderValue() style to render the specific values (for example "drip" should be dark/italic, not plain grey text), we might need to add a renderValueList() or something:

Screen Shot 2017-05-19 at 9.06.34 AM.png (134×457 px, 22 KB)

I think this is pre-existing, but our behavior when you enter a bogus hashtag isn't great. Currently, we normalize it to the closest acceptable hashtag, so if you enter !!!X!!! you get #x. Better would probably be raising an error?

When editing the project picture, I get sent to different places depending on how I do it:

  • Clicking "Use Picture" sends me back to the management page.
  • Uploading a picture, composing a picture, or clicking "Current Picture" sends me back to the profile page.
  • Probably, all routes should go to the manage page.

The "Disable Mail" and "Enable Mail" actions have the same icon. This isn't really a problem but is slightly unusual.

Design on this page is slightly un-modern I think (missing big header + blue header)? I am really just grasping at straws here

Screen Shot 2017-05-19 at 10.47.37 AM.png (873×1 px, 142 KB)

Basically I can't find anything broken in projects.

Probably pre-existing, but in Pholio, mentioning users in the description does not subscribe them. Embedding images in the description does not attach them to the mock properly. This suggests that the description is not being returned from some getRemarkupBlocks() sort of method although I don't remember which offhand.

Almost certainly pre-existing, but mentioning users in a Pholio image description doesn't subscribe them. Same deal with adding images there.

Pre-existing: you can't drag-and-drop images into a pholio image description (they update the mock image instead).

Pre-existing: closed mocks aren't visually distinct from open mocks on the pinboard? OPPORTUNITY FOR GREYSCALE!?

Probably pre-existing (?): adding a inline comment to an older version of an image doesn't cause the comment to be submitted when you submit the comment form.

From a language consistency viewpoint, these could maybe drop the word "mock", but they read fine to me as-is:

Screen Shot 2017-05-19 at 11.06.54 AM.png (197×460 px, 27 KB)

This one could maybe be "unlocked membership for X"?

Screen Shot 2017-05-19 at 11.07.22 AM.png (110×353 px, 11 KB)

But this basically all amounts to "Pholio could use an iteration", I didn't actually find anything substantive.

Instances:

  • Maybe drop "Created" property? Doesn't seem terribly useful to me. This might have been a placeholder at one time to make sure that we always had something so the UI didn't render weird, but I think there's now no case where we have no properties. Fine to keep if you just like having it.
  • The list has lost icon colors. Maybe intentional? Makes things a little harder to pick out visually to me:

Screen Shot 2017-05-19 at 11.20.49 AM.png (335×432 px, 33 KB)

  • "up" is also no longer green, but that's probably on purpose?
  • Pre-existing, but maybe we should label "Up" as "Running" or something in the UI for clarity now that it's more prominent.
  • Pre-existing, but "enable instance" is incorrectly rendered as disabled even when you can do it (because you're an admin / manager)?

The only Maniphest thing I've caught is this:

Screen Shot 2017-05-19 at 11.26.10 AM.png (154×451 px, 17 KB)

Maybe clearer as "alice changed the title of Txxx from X to Y."

In Phriction:

  • Pre-existing, but if you enter edit notes without making edits and submit, your notes are lost. Ideally, we'd raise an error ("you didn't make any changes but provided notes").
  • Actual Bug Moving a document from /x/ to /y/ doesn't remove the document from /x/, it just copies it to /y/.
  • You can move a document from /aaa/ to /aaa/bbb/, but we don't let you create a document directly at /aaa/bbb/ even though we end up in the same state. This is probably pre-existing.

So I found one actual bug, I think.

I'm planning to fix these, since I think they're easy or actually annoying, and just let everything else sit until we iterate or someone else notices:

  • Hashtag transactions don't render as nicely as they could.
  • Project picture stuff redirects inconsistently.
  • Pholio subscribers/file attachment.
  • Project "membership" string.
  • Drop "Created" property for instances.
  • Instance list icon colors, unless that was intentional?
  • Instance "Enable" state being incorrectly disabled for users with management permission.
chad added a revision: Restricted Differential Revision.May 19 2017, 9:02 PM
chad added a commit: Restricted Diffusion Commit.May 19 2017, 9:08 PM
chad added a revision: Restricted Differential Revision.May 19 2017, 9:14 PM
chad added a commit: Restricted Diffusion Commit.May 19 2017, 9:16 PM

Drop "Created" property for instances.

I think it's nice to see. Any specific reason?

oh property. yeah, thought you meant transaction.

I'm fine with keeping it, just thought it might be a legacy holdover from a bygone era.

Oh, yeah, transaction is great. Property feels a little clutter-y since we have the transaction anyway.

chad added a revision: Restricted Differential Revision.May 19 2017, 9:20 PM
epriestley claimed this task.

If anyone wants to fix anything else here feel free, but I think we squeezed most of the soup from this stone once D17972 lands.

@chad / @amckinley Nice job with these changes!

chad added a commit: Restricted Diffusion Commit.May 19 2017, 9:41 PM