Page MenuHomePhorge

Project Hovercards: Show Description
ClosedPublic

Authored by avivey on Jul 4 2023, 15:52.

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.