Adds a basic "Revision {$num}" table header
Details
- Reviewers
epriestley - Maniphest Tasks
- T5383: Add additional hints to image history table in Pholio
- Commits
- Restricted Diffusion Commit
rPfc2588d6e20d: Add table headers to Pholio Mock History table
Actually spend more time testing various depth layouts.
Diff Detail
- Repository
- rP Phabricator
- Branch
- pholio-revision
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 1275 Build 1275: [Placeholder Plan] Wait for 30 Seconds
Event Timeline
The possible issue I see here is that if you add images A and B, then update just A, you get:
Revision 2 | A2 | B1 Revision 1 | A1 |
Here, B1 is labeled "Revision 2", even though it hasn't been revised. Do you think that's fine?
Another point of confusion is that if you then delete B1, add C1, and update A2 twice, you get this (roughly):
Revision 4 | A4 | | C1 Revision 3 | A3 | B1 | Revision 2 | A2 | | Revision 1 | A1 | |
However, a layout where columns don't "float to the top" might make the update history more clear:
Revision 4 | A4 | | Revision 3 | A3 | | Revision 2 | A2 | | C1 Revision 1 | A1 | B1 |
Or we could float the current status or something:
Current | A4 | XX | C1 Revision 4 | A4 | : | : Revision 3 | A3 | : | : Revision 2 | A2 | : | C1 Revision 1 | A1 | B1 |
I'm not sure if doing any of this is useful or interesting. If we do want to do any of this, we need to make backend changes to how the data is stored, since we don't currently record what overall version of a mock things changed at.
I think it's also fine to leave things as they are, but explicitly labeling versions might make this more confusing than it is right now, and I'm not sure if we want to plan to fix this stuff. Tentatively, I'd say;
- ship this;
- see if anyone complains or has a hard time figuring it out?
...but maybe you have more far-reaching thoughts.
The main thing for me was giving people something to easily reference in discussions. "I felt the direction in Revision 3 was more on target, Revision 4 just adds details and features we don't need". Then people can click on one of the images in that History Table and tab left and right to review them.
We could, if needed, add additional hints as to what happened between Revision 1 and Revision two, maybe a small icon for (added|removed|updated). I can explore that in another diff.
I felt the direction in Revision 3 was more on target, Revision 4 just adds details and features we don't need
I think this doesn't do that. Suppose I add A1 and B1, then update them:
Revision 2 | A2 | B2 | Revision 1 | A1 | B1 |
So far, so good. Someone leaves a comment on B2 like "I preferred the color in revision 1".
Now, just A2 gets updated. The table looks like this, because columns "float to the top":
Revision 3 | A3 | B2 | Revision 2 | A2 | B1 | Revision 1 | A1 | |
The comment on B2 that someone preferred the color in "revision 1" has now lost context -- there's no version of B at "Revision 1" anymore.
Some possible approaches...
- Ship it for now like this and see how confusing it is?
- Put version bubbles on the images in each column, like v1, v2 -- per-column, the numbers are stable.
- Stabilize the table (stop things from floating to the top): the backend work to make the table stable isn't too hard, I just don't want do it if we don't actually need/want it. (I think having a ragged top row may be a little weird, too, but that has other possible fixes).
- Some combination of this stuff?
Yeah, I think my expectation was that "Revision 1" is everything that existed in the mock set at that time, I didn't catch on that things were bubbling up (though, that makes for more duplication, but that's probably not super crazy).
I think I'd rather just go simple, label the top set as 'Current Revision' and wait for feedback / actual use for improving or adding more information to the history table. It's not clear how useful anything else there would be.
In my head I'm leaning towards a Timeline model, like PHUITimeline, so it'd show a line between images with icons as to what happend (added / removed / deleted).
Seems like a reasonable compromise.
(If we get confusion/feedback on this I'm happy to take another look at the backend parts. We could also scrap the "grid" idea entirely, but I think I like it. Maybe it will be less useful/intuitive/valuable with real data than it is with Pokemon, though.)