Page MenuHomePhabricator

Fix "Actions" button on Phame standalone/live pages (bonus: JX.sprintf())
ClosedPublic

Authored by epriestley on Apr 3 2019, 5:50 PM.
Tags
None
Referenced Files
F14405965: D20378.diff
Mon, Dec 23, 9:27 AM
Unknown Object (File)
Tue, Dec 17, 4:33 PM
Unknown Object (File)
Wed, Dec 11, 11:27 PM
Unknown Object (File)
Wed, Dec 11, 7:12 AM
Unknown Object (File)
Tue, Dec 10, 4:23 PM
Unknown Object (File)
Fri, Dec 6, 6:59 AM
Unknown Object (File)
Tue, Dec 3, 9:17 AM
Unknown Object (File)
Sat, Nov 30, 9:17 AM
Subscribers
None

Details

Summary

See https://discourse.phabricator-community.org/t/non-functional-actions-menu-on-live-phame-views/2593. Several layers here:

The "Actions" button is broken because a menu behavior is failing, since we aren't rendering the menu.

When a behavior fails to initialize, catch and log the exception and continue. Previously, we stopped initializing behaviors if any failed, but behaviors are usually independent and continuing with an explicit exception seems reasonable.

Give "JX.log()" some "sprintf()" semantics to make logging the behavior failure easier. We can probably afford these extra 200 bytes now in 2019.

This fixes the button and gives us explicit errors in the log. So far, so good.

Then, when a page won't render chrome, don't try to render the main menu. This fixes the actual errors (we no longer try to initialize menu behaviors for nodes which don't exist).

Completely hide the "Actions" and "Comment" flows if the viewer isn't logged in. Although this isn't completely consistent with other applications, I think it's more appropriate for Phame. In applications like Maniphest, we show a full set of controls (but disable them) so that users who are not currently logged in have a clear path to interact with the content, under the assumption that this is a relatively common workflow. This is probably less common for Phame, where we expect most anonymous viewers not to log in or interact.

Finally, parametrize a one-off border color and add a border under the crumbs at the top of the page.

Test Plan
  • Viewed a "Live" Phame blog post page, clicked "Actions", got a dropdown.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

amckinley added inline comments.
webroot/rsrc/externals/javelin/core/util.js
306–360

What is this; a Facebook interview?!?

344

Wow, charAt returns the empty string instead of null or something?

This revision is now accepted and ready to land.Apr 3 2019, 7:39 PM
webroot/rsrc/externals/javelin/core/util.js
344

You betcha!

>> ''.charAt(420.69)
<< ""

You might already know this one, too:

>> typeof null
<< "object"

Javascript is basically worse than PHP but has a much better PR team.

This revision was automatically updated to reflect the committed changes.