Skip to content

Commit 248d79f

Browse files
author
epriestley
committedApr 4, 2019
Fix "Actions" button on Phame standalone/live pages (bonus: JX.sprintf())
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. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D20378
1 parent 18732a0 commit 248d79f

File tree

8 files changed

+91
-15
lines changed

8 files changed

+91
-15
lines changed
 

‎resources/celerity/map.php

+11-11
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
'conpherence.pkg.css' => '3c8a0668',
1111
'conpherence.pkg.js' => '020aebcf',
1212
'core.pkg.css' => 'a1c2d49b',
13-
'core.pkg.js' => 'a747b035',
13+
'core.pkg.js' => 'a568e834',
1414
'differential.pkg.css' => '8d8360fb',
1515
'differential.pkg.js' => '67e02996',
1616
'diffusion.pkg.css' => '42c75c37',
@@ -86,7 +86,7 @@
8686
'rsrc/css/application/paste/paste.css' => 'b37bcd38',
8787
'rsrc/css/application/people/people-picture-menu-item.css' => 'fe8e07cf',
8888
'rsrc/css/application/people/people-profile.css' => '2ea2daa1',
89-
'rsrc/css/application/phame/phame.css' => '799febf9',
89+
'rsrc/css/application/phame/phame.css' => 'bb442327',
9090
'rsrc/css/application/pholio/pholio-edit.css' => '4df55b3b',
9191
'rsrc/css/application/pholio/pholio-inline-comments.css' => '722b48c2',
9292
'rsrc/css/application/pholio/pholio.css' => '88ef5ef1',
@@ -217,7 +217,7 @@
217217
'rsrc/externals/javelin/core/init.js' => '98e6504a',
218218
'rsrc/externals/javelin/core/init_node.js' => '16961339',
219219
'rsrc/externals/javelin/core/install.js' => '5902260c',
220-
'rsrc/externals/javelin/core/util.js' => '22ae1776',
220+
'rsrc/externals/javelin/core/util.js' => 'edb4d8c9',
221221
'rsrc/externals/javelin/docs/Base.js' => '5a401d7d',
222222
'rsrc/externals/javelin/docs/onload.js' => 'ee58fb62',
223223
'rsrc/externals/javelin/ext/fx/Color.js' => '78f811c9',
@@ -259,7 +259,7 @@
259259
'rsrc/externals/javelin/lib/__tests__/JSON.js' => '710377ae',
260260
'rsrc/externals/javelin/lib/__tests__/URI.js' => '6fff0c2b',
261261
'rsrc/externals/javelin/lib/__tests__/behavior.js' => '8426ebeb',
262-
'rsrc/externals/javelin/lib/behavior.js' => 'fce5d170',
262+
'rsrc/externals/javelin/lib/behavior.js' => '1b6acc2a',
263263
'rsrc/externals/javelin/lib/control/tokenizer/Tokenizer.js' => '89a1ae3a',
264264
'rsrc/externals/javelin/lib/control/typeahead/Typeahead.js' => 'a4356cde',
265265
'rsrc/externals/javelin/lib/control/typeahead/normalizer/TypeaheadNormalizer.js' => 'a241536a',
@@ -571,7 +571,7 @@
571571
'herald-test-css' => 'e004176f',
572572
'inline-comment-summary-css' => '81eb368d',
573573
'javelin-aphlict' => '022516b4',
574-
'javelin-behavior' => 'fce5d170',
574+
'javelin-behavior' => '1b6acc2a',
575575
'javelin-behavior-aphlict-dropdown' => 'e9a2940f',
576576
'javelin-behavior-aphlict-listen' => '4e61fa88',
577577
'javelin-behavior-aphlict-status' => 'c3703a16',
@@ -729,7 +729,7 @@
729729
'javelin-typeahead-source' => '8badee71',
730730
'javelin-typeahead-static-source' => '80bff3af',
731731
'javelin-uri' => '2e255291',
732-
'javelin-util' => '22ae1776',
732+
'javelin-util' => 'edb4d8c9',
733733
'javelin-vector' => 'e9c80beb',
734734
'javelin-view' => '289bf236',
735735
'javelin-view-html' => 'f8c4e135',
@@ -798,7 +798,7 @@
798798
'phabricator-tooltip' => '83754533',
799799
'phabricator-ui-example-css' => 'b4795059',
800800
'phabricator-zindex-css' => '99c0f5eb',
801-
'phame-css' => '799febf9',
801+
'phame-css' => 'bb442327',
802802
'pholio-css' => '88ef5ef1',
803803
'pholio-edit-css' => '4df55b3b',
804804
'pholio-inline-comments-css' => '722b48c2',
@@ -1029,6 +1029,10 @@
10291029
'javelin-stratcom',
10301030
'javelin-util',
10311031
),
1032+
'1b6acc2a' => array(
1033+
'javelin-magical-init',
1034+
'javelin-util',
1035+
),
10321036
'1c850a26' => array(
10331037
'javelin-install',
10341038
'javelin-util',
@@ -2175,10 +2179,6 @@
21752179
'phabricator-keyboard-shortcut',
21762180
'conpherence-thread-manager',
21772181
),
2178-
'fce5d170' => array(
2179-
'javelin-magical-init',
2180-
'javelin-util',
2181-
),
21822182
'fdc13e4e' => array(
21832183
'javelin-install',
21842184
),

‎src/applications/celerity/postprocessor/CelerityDefaultPostprocessor.php

+1
Original file line numberDiff line numberDiff line change
@@ -238,6 +238,7 @@ public function buildVariables() {
238238
'grey.button.gradient' => 'linear-gradient(to bottom, #ffffff, #f1f0f1)',
239239
'grey.button.hover' => 'linear-gradient(to bottom, #ffffff, #eeebec)',
240240

241+
'document.border' => '#dedee1',
241242

242243
);
243244
}

‎src/applications/phame/controller/post/PhamePostViewController.php

+2-2
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ public function handleRequest(AphrontRequest $request) {
2424

2525
$hero = $this->buildPhamePostHeader($post);
2626

27-
if (!$is_external) {
27+
if (!$is_external && $viewer->isLoggedIn()) {
2828
$actions = $this->renderActions($post);
2929
$header->setPolicyObject($post);
3030
$header->setActionList($actions);
@@ -136,7 +136,7 @@ public function handleRequest(AphrontRequest $request) {
136136
->withTransactionTypes(array(PhabricatorTransactions::TYPE_COMMENT)));
137137
$timeline->setQuoteRef($monogram);
138138

139-
if ($is_external) {
139+
if ($is_external || !$viewer->isLoggedIn()) {
140140
$add_comment = null;
141141
} else {
142142
$add_comment = $this->buildCommentForm($post, $timeline);

‎src/view/page/PhabricatorStandardPageView.php

+7
Original file line numberDiff line numberDiff line change
@@ -304,6 +304,12 @@ protected function willRenderPage() {
304304
));
305305
}
306306

307+
// If we aren't showing the page chrome, skip rendering DarkConsole and the
308+
// main menu, since they won't be visible on the page.
309+
if (!$this->getShowChrome()) {
310+
return;
311+
}
312+
307313
if ($console) {
308314
require_celerity_resource('aphront-dark-console-css');
309315

@@ -347,6 +353,7 @@ protected function willRenderPage() {
347353
$menu->setApplicationMenu($application_menu);
348354
}
349355

356+
350357
$this->menuContent = $menu->render();
351358
}
352359

‎webroot/rsrc/css/application/phame/phame.css

+1
Original file line numberDiff line numberDiff line change
@@ -284,6 +284,7 @@
284284
text-align: center;
285285
background: {$page.content};
286286
padding: 16px 0 24px;
287+
border-top: 1px solid {$document.border};
287288
}
288289

289290
.device-phone .phame-mega-header {

‎webroot/rsrc/css/phui/phui-document-pro.css

+1-1
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@
5656
.phui-document-container {
5757
background-color: {$page.content};
5858
position: relative;
59-
border-bottom: 1px solid #dedee1;
59+
border-bottom: 1px solid {$document.border};
6060
}
6161

6262
.phui-document-view-pro-box,

‎webroot/rsrc/externals/javelin/core/util.js

+60
Original file line numberDiff line numberDiff line change
@@ -295,9 +295,69 @@ if (!window.console || !window.console.log) {
295295
* @return void
296296
*/
297297
JX.log = function(message) {
298+
// "JX.log()" accepts "Error" in addition to "string". Only try to
299+
// treat the argument as a "sprintf()" pattern if it's a string.
300+
if (typeof message === 'string') {
301+
message = JX.sprintf.apply(null, arguments);
302+
}
298303
window.console.log(message);
299304
};
300305

306+
JX.sprintf = function(pattern) {
307+
var argv = Array.prototype.slice.call(arguments);
308+
argv.reverse();
309+
310+
// Pop off the pattern argument.
311+
argv.pop();
312+
313+
var len = pattern.length;
314+
var output = '';
315+
for (var ii = 0; ii < len; ii++) {
316+
var c = pattern.charAt(ii);
317+
318+
if (c !== '%') {
319+
output += c;
320+
continue;
321+
}
322+
323+
ii++;
324+
325+
var next = pattern.charAt(ii);
326+
if (next === '%') {
327+
// This is "%%" (that is, an escaped "%" symbol), so just add a literal
328+
// "%" to the result.
329+
output += '%';
330+
continue;
331+
}
332+
333+
if (next === 's') {
334+
if (!argv.length) {
335+
throw new Error(
336+
'Too few arguments to "JX.sprintf(...)" for pattern: ' + pattern);
337+
}
338+
339+
output += '' + argv.pop();
340+
341+
continue;
342+
}
343+
344+
if (next === '') {
345+
throw new Error(
346+
'Pattern passed to "JX.sprintf(...)" ends with "%": ' + pattern);
347+
}
348+
349+
throw new Error(
350+
'Unknown conversion "%' + c + '" passed to "JX.sprintf(...)" in ' +
351+
'pattern: ' + pattern);
352+
}
353+
354+
if (argv.length) {
355+
throw new Error(
356+
'Too many arguments to "JX.sprintf()" for pattern: ' + pattern);
357+
}
358+
359+
return output;
360+
};
301361

302362
if (__DEV__) {
303363
window.alert = (function(native_alert) {

‎webroot/rsrc/externals/javelin/lib/behavior.js

+8-1
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,14 @@ JX.initBehaviors = function(map) {
9191
configs = [null];
9292
}
9393
for (var ii = 0; ii < configs.length; ii++) {
94-
JX.behavior._behaviors[name](configs[ii], JX.behavior._statics[name]);
94+
try {
95+
JX.behavior._behaviors[name](configs[ii], JX.behavior._statics[name]);
96+
} catch (behavior_exception) {
97+
JX.log(
98+
'JX.initBehaviors(...): behavior "%s" raised an error during setup.',
99+
name);
100+
JX.log(behavior_exception);
101+
}
95102
}
96103
JX.behavior._initialized[name] = true;
97104
}

0 commit comments

Comments
 (0)
Failed to load comments.