Details

Reviewers

Group Reviewers

Commits

rP4f3aad4f9f21: Portals: Rename unused newNoMenuItemsView() to newNoContentView()

Summary

Rename uncalled newNoMenuItemsView() to newNoContentView() overwriting this method in the parent class, to fix showing more specific UI messages for empty portals.

Test Plan

Code:

Grep the code for non-existing newNoMenuItemsView
Check code of parent class PhabricatorProfileMenuEngine

Workflows:

Create an empty portal via http://phorge.localhost/portal/edit/form/default/, see correct (more specific) string "Use "Edit Menu" to add menu items to this portal." displayed
Click "Edit Menu" to go to http://phorge.localhost/portal/view/1/configure/global/ of that Portal just created, on the right in "Add New Menu Item..." select "Divider" and click "Create Menu Item" button, go back to portal, see correct (more specific) string "None of the visible menu items in this portal can render any content." displayed

Diff Detail

Repository

rP Phorge

Branch

portalProfileMenu

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 1526
Build 1526: arc lint + arc unit

Event Timeline

aklapper created this revision.Aug 14 2024, 12:58

Owners added a reviewer: O1: Blessed Committers.Aug 14 2024, 12:58

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptAug 14 2024, 12:58

aklapper requested review of this revision.Aug 14 2024, 12:58

Harbormaster completed remote builds in B1526: Diff 2369.Aug 14 2024, 12:58

Cigaryno added a project: Dashboards.Dec 29 2024, 16:40

valerio.bozzolan requested changes to this revision.Feb 2 2025, 15:58

valerio.bozzolan added inline comments.

src/applications/dashboard/engine/PhabricatorDashboardPortalProfileMenuEngine.php
35	Interestingly, the method `newNoMenuItemsView()` seems orphan! $ grep -RF -- '->newNoMenuItemsView' . no results So probably we can just drop this method

This revision now requires changes to proceed.Feb 2 2025, 15:58

aklapper added inline comments.Feb 3 2025, 10:46

src/applications/dashboard/engine/PhabricatorDashboardPortalProfileMenuEngine.php
35	Uh right, thanks, seems https://we.phorge.it/rPdfe47157d322d3500f5eee204672cdb3d2c32c16 removed all except for this one, and creating an empty portal shows the strings defined in src/applications/search/engine/PhabricatorProfileMenuEngine.php instead.

aklapper retitled this revision from Fix newEmptyView() parameters in PhabricatorDashboardPortalProfileMenuEngine to Remove unused newNoMenuItemsView() function.Feb 3 2025, 10:49

aklapper edited the summary of this revision. (Show Details)

aklapper edited the test plan for this revision. (Show Details)

Remove newNoMenuItemsView() instead

Harbormaster completed remote builds in B1677: Diff 2611.Feb 3 2025, 10:49

aklapper retitled this revision from Fix newEmptyView() parameters in PhabricatorDashboardPortalProfileMenuEngine to Remove unused newNoMenuItemsView() function.Feb 3 2025, 10:50

aklapper edited the summary of this revision. (Show Details)

aklapper edited the test plan for this revision. (Show Details)

aklapper marked an inline comment as done.

valerio.bozzolan added inline comments.Feb 4 2025, 13:27

src/applications/dashboard/engine/PhabricatorDashboardPortalProfileMenuEngine.php
35	Last comment promise... but I discovered that renaming to `newNoContentView()` ... renders something! with: protected function newNoContentView(array $items) { $object = $this->getProfileObject(); $builtins = $this->getBuiltinProfileItems($object); if (count($items) <= count($builtins)) { return $this->newEmptyView( pht('New Portal'), pht('Use "Edit Menu" to add menu items to this portal.')); } else { return $this->newEmptyView( pht('No Portal Content'), pht( 'None of the visible menu items in this portal can render any '. 'content.'), ); } } With: line 35 from `newNoMenuItemsView` to `newNoContentView` line `newNoContentView` to be adjusted to avoid `newNoContentView()` (to avoid recursions) In this case the test plan would be: having a portal with nothing: renders first message having a portal with just one "Divider": renders second message having a portal with something useful (e.g. a Dashboard): shows the Dashboard

aklapper edited the test plan for this revision. (Show Details)Feb 16 2025, 21:10

Right, PhabricatorDashboardPortalProfileMenuEngine extends PhabricatorProfileMenuEngine so we can override newNoContentView()... that probably makes most sense, thanks

Rename function to override from parent class, as proposed by Valerio

Harbormaster completed remote builds in B1713: Diff 2657.Feb 16 2025, 21:19

Run ./bin/celerity map

Harbormaster completed remote builds in B1714: Diff 2658.Feb 16 2025, 21:22

having a portal with something useful (e.g. a Dashboard): shows the Dashboard

I have no freaking idea how to make portals show something useful, and I never understood the difference between portals and dashboards anyway, so I think I don't care. :)

aklapper marked an inline comment as done.Feb 16 2025, 21:30

As I uploaded a new diff now how do I request a new review / reset the existing one "Changes Requested"?
"Foist Upon", or which vaguely named oh so "funny" option from the "Add Action..." dropdown which makes non-native speakers feel stupid and be scared to choose the wrong option? Oh UX and usability...

Let's try again without celerity noise

Harbormaster completed remote builds in B1718: Diff 2663.Feb 16 2025, 23:51

Okay, got to wait until D25885 is merged first

Test plan works! Thanks :3

lgtm