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
Details
- Reviewers
valerio.bozzolan avivey - Group Reviewers
O1: Blessed Committers
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
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 |
src/applications/maniphest/application/PhabricatorManiphestApplication.php | ||
---|---|---|
112 | 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
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 |
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. |
Please wait more feedback from avivey, but I wanted to say that I tested this and I love it