Page MenuHomePhorge

Show more in application detail and list view
AcceptedPublic

Authored by Matthew on Jul 28 2023, 15:32.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Sep 21, 05:53
Unknown Object (File)
Sat, Sep 16, 13:48
Unknown Object (File)
Thu, Sep 14, 03:41
Unknown Object (File)
Sun, Sep 3, 10:24
Unknown Object (File)
Sat, Sep 2, 11:36
Unknown Object (File)
Thu, Aug 31, 07:36
Unknown Object (File)
Tue, Aug 29, 20:15
Unknown Object (File)
Sat, Aug 26, 10:34
Tokens
"Like" token, awarded by avivey."Love" token, awarded by valerio.bozzolan.

Details

Summary

Update the application detail view and list view to show a unified set of badges, show PHIDs and monograms on the application detail view, allow applications to register monograms

T15568

Test Plan

Loaded the UI in a local test phorge, see screenshots

Diff Detail

Repository
rP Phorge
Branch
arcpatch-D25362
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 713
Build 713: arc lint + arc unit

Event Timeline

Matthew requested review of this revision.Jul 28 2023, 15:32

Here are some screenshots of this change in action:

SCR-20230728-icuv.png (67×1 px, 15 KB)

SCR-20230728-iecv.png (1×2 px, 719 KB)

SCR-20230728-iryi.png (1×2 px, 741 KB)

Matthew retitled this revision from Update the application detail view and list view to show a unified set of badges, allow applications to register monograms to Update the application detail view and list view to show a unified set of badges, show PHIDs and monograms on the application detail view, allow applications to register monograms.Jul 28 2023, 15:39

Uh nice!

src/applications/maniphest/application/PhabricatorManiphestApplication.php
112

OH NO AN IMPORTANT SPACE WAS REMOVED

src/applications/owners/application/PhabricatorOwnersApplication.php
52

Probably just one newline is nice

src/applications/phid/type/PhabricatorPHIDType.php
216

Please attach the PHP doc in the bottom, not in the top

Matthew added inline comments.
src/applications/maniphest/application/PhabricatorManiphestApplication.php
112

Macro notmyfault:

This was actually probably automatically done by my IDE.

I'm not really opinionated in indentation but it seems the majority of the lines are indented with 4 spaces instead of two

src/applications/meta/controller/PhabricatorApplicationDetailViewController.php
41–47

This also probably has an indentation problem

(And the lint of course does not says anything)

48–64

it seems Phorge indents with two spaces

(And the lint of course does not says anything)

123

2 spaces

Unfortunately, PHPStorm defaults to four spaces. I'm not really willing to spend a ton of time arguing with it (especially since my day job requires four spaces) ... though it would be helpful if we could come up with some clear code rules and them implement them in the repository using phpcs.

Can I amend this change fixing spaces? I have some time to waste right now and I love this kind of "del del del" ihih

Can I amend this change fixing spaces? I have some time to waste right now and I love this kind of "del del del" ihih

Feel free!

src/applications/meta/controller/PhabricatorApplicationDetailViewController.php
138

Can I also amend here just an if ($monograms) ?

src/applications/meta/controller/PhabricatorApplicationDetailViewController.php
138

Sure!

I thought we had "2 spaces" and the rest encoded as lints... I'll try to take a look.

src/applications/base/PhabricatorApplication.php
65

Extension != application, although they often do.

An extension might only have Engine Extensions and stuff.

Also - maybe adding getPHIDclass() or getApplicationClass() to PhabricatorObjectRemarkupRule, and then running the search in reverse will make it easier for application authors?

src/applications/meta/controller/PhabricatorApplicationDetailViewController.php
123

Also, if we're being pedantic, each argument is supposed to be in a separate line.

130

this can be just if ($user_firendly_phids), because php.

src/applications/phid/type/PhabricatorPHIDType.php
220

installed is confusing here

avivey retitled this revision from Update the application detail view and list view to show a unified set of badges, show PHIDs and monograms on the application detail view, allow applications to register monograms to Show more in application detail and list view .Jul 28 2023, 16:55
avivey edited the summary of this revision. (Show Details)
avivey awarded a token.
Matthew retitled this revision from Show more in application detail and list view to Show more in application detail and list view.

Address code review comments

src/applications/base/PhabricatorApplication.php
65

Yes, that wording was intentional. Extensions are allowed to register multi-character monograms, applications (internal) will not. Down the road, I will be using this overridden method to do the check for monogram conflicts.

As for using the Remarkup rule, see what you mean. I went with this because it puts the burden on the application author to register and track their monograms. Also, it opens the door for an extension to register monograms without a remarkup rule, though I couldn't think of a situation where that could happen.

Soft-accept! Tested and it works. Waiting for another tester.

Please wait more feedback from avivey, but I wanted to say that I tested this and I love it

This revision is now accepted and ready to land.Aug 15 2023, 08:02