Page MenuHomePhorge

Project Hovercards: Show Description
ClosedPublic

Authored by avivey on Jul 4 2023, 15:52.
Tags
None
Referenced Files
F3328366: D25331.1743455261.diff
Sun, Mar 30, 21:07
F3321918: D25331.1743362958.diff
Sat, Mar 29, 19:29
F3308225: D25331.1743160930.diff
Thu, Mar 27, 11:22
F3305737: D25331.1743119250.diff
Wed, Mar 26, 23:47
F3300767: D25331.1743046576.diff
Wed, Mar 26, 03:36
F3294676: D25331.1742941198.diff
Mon, Mar 24, 22:19
F3294257: D25331.1742931257.diff
Mon, Mar 24, 19:34
F3252647: D25331.1742397376.diff
Tue, Mar 18, 15:16

Details

Summary

Fixes T15275.

Test Plan

Project pages have "Developer > View Hovercard" button. Or hover over a project handle, I guess.

Also test for empty description and with the description field disabled.

Diff Detail

Repository
rP Phorge
Branch
master
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 636
Build 636: arc lint + arc unit

Event Timeline

avivey requested review of this revision.Jul 4 2023, 15:52
src/applications/project/engineextension/PhabricatorProjectHovercardEngineExtension.php
37–40

I thought the intention here was to skip just the current disabled one, so I'm surprised about the break instead of continue.

Maybe a better comment to clarify?

// The field is disabled, and also the others would always be null.
src/applications/project/view/PhabricatorProjectCardView.php
78

Should we dedicate another variable for this semantically different value, instead of replacing the original one?

src/applications/project/engineextension/PhabricatorProjectHovercardEngineExtension.php
37–40

It's always the same field... we only have one...
Would it be cleared if it was continue? It's more-or-less meaningless from the POV of performance (I'm not sure there's any non-test way to get into this loop with more then one object...)

src/applications/project/view/PhabricatorProjectCardView.php
78

we often re-purpose variables when we don't need the old value any more. Semantically they're the same, they are only different on a technical level.

This is equivalent of doing

$msg = 'foo';
$msg .= 'bar';

A person enters in a bar and

  • ✅ ... orders a one-line description
  • ✅ ... orders a multi-line description
  • ✅ ... orders no description
  • 🔶 ... orders a lovely "cowsay {{{ asd }}}"

Cowsay descriptioned.png (258×422 px, 13 KB)

src/applications/project/engineextension/PhabricatorProjectHovercardEngineExtension.php
37–40

Maybe we can use $project = head($projects) to avoid to work inside a loop and just operate on the first element

src/applications/project/engineextension/PhabricatorProjectHovercardEngineExtension.php
37–40

no, the projects are different, the field instances are different, fields definitions are the same.

Each project instance needs its own field instance.

src/applications/project/engineextension/PhabricatorProjectHovercardEngineExtension.php
37–40

So in theory we should also reset $custom_fields if we find a NULL $field but in that case the $custom_fields is already empty so a break is enough since it always will happen in the first cycle. OK.

src/applications/project/engineextension/PhabricatorProjectHovercardEngineExtension.php
37–40

Again maybe a comment like

// The description is disabled for this Project, but also for any other.

It is possible to encounter a couple of problems but that plague any other Hovercard. Thanks! This is useful.

Note for other people: this shows a one-line, not everything. So the Hovercard is still very small.

lgtm

This revision is now accepted and ready to land.Jul 5 2023, 13:57
This revision was automatically updated to reflect the committed changes.