2011-01-31 20:38:13 -08:00
|
|
|
/**
|
|
|
|
|
* @provides javelin-behavior-differential-show-more
|
Bring Javelin into Phabricator via git submodule, not copy-and-paste
Summary:
Javelin is currently embedded in Phabricator via copy-and-paste of prebuilt
packages. This is not so great.
Pull it in as a submodule instead and make all the Phabriator resources declare
proper dependency trees. Add Javelin linting.
Test Plan:
I tried to run through pretty much all the JS functionality on the site. This is
still a high-risk change, but I did a pretty thorough test
Differential: inline comments, revealing diffs, list tokenizers, comment
preview, editing/deleting comments, add review action.
Maniphest: list tokenizer, comment actions
Herald: rule editing, tokenizers, add/remove rows
Reviewed By: tomo
Reviewers: aran, tomo, mroch, jungejason, tuomaspelkonen
CC: aran, tomo, epriestley
Differential Revision: 223
2011-05-03 15:11:55 -07:00
|
|
|
* @requires javelin-behavior
|
|
|
|
|
* javelin-dom
|
2011-07-16 07:09:19 -07:00
|
|
|
* javelin-workflow
|
Bring Javelin into Phabricator via git submodule, not copy-and-paste
Summary:
Javelin is currently embedded in Phabricator via copy-and-paste of prebuilt
packages. This is not so great.
Pull it in as a submodule instead and make all the Phabriator resources declare
proper dependency trees. Add Javelin linting.
Test Plan:
I tried to run through pretty much all the JS functionality on the site. This is
still a high-risk change, but I did a pretty thorough test
Differential: inline comments, revealing diffs, list tokenizers, comment
preview, editing/deleting comments, add review action.
Maniphest: list tokenizer, comment actions
Herald: rule editing, tokenizers, add/remove rows
Reviewed By: tomo
Reviewers: aran, tomo, mroch, jungejason, tuomaspelkonen
CC: aran, tomo, epriestley
Differential Revision: 223
2011-05-03 15:11:55 -07:00
|
|
|
* javelin-util
|
|
|
|
|
* javelin-stratcom
|
2011-01-31 20:38:13 -08:00
|
|
|
*/
|
|
|
|
|
|
|
|
|
|
JX.behavior('differential-show-more', function(config) {
|
|
|
|
|
|
2012-01-05 20:29:16 -08:00
|
|
|
function onresponse(context, response) {
|
Don't mangle inline comments with tables in them in Differential
Summary:
Fixes T3814. Broadly, remarkup tables in inline comments did not work properly. I ran into several messes here and cleaned up some of them:
- Some of this code is doing `JX.$N('div', {}, JX.$H(response.markup))`, to turn an HTML response into a node, passing that around, and then doing junk with it. This is super old and gross.
- The slightly more modern pattern is `JX.$H(response.markup).getFragment().firstChild`, but this is kind of yuck too and not as safe as it could be.
- Introduce `JX.$H(response.markup).getNode()`, which actually expresses intent here. We have a bunch of `getFragment().firstChild` callsites which should switch to this, but I didn't clean those up yet because I don't want to test them all.
- Switch the `JX.$N('div', {}, JX.$H(response.markup))`-style callsites to `JX.$H(response.markup).getNode()`.
- `copyRows()` is too aggressive in finding `<tr />` tags. This actually causes the bug in T3814. We only want to find these tags at top level, not all tags. Don't copy `<tr />` tags which belong to some deeper table.
- Once this is fixed, there's another bug with mousing over the cells in tables in inline comments. We select the nearest `<td />`, but that's the cell in the remarkup table. Instead, select the correct `<td />`.
- At this point, these last two callsites were looking ugly. I provided `findAbove()` to clean them up.
Test Plan: Created, edited, deleted, moused over, and reloaded a revision with inline comments including remarkup tables. Used "Show more context" links.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3814
Differential Revision: https://secure.phabricator.com/D6924
2013-09-10 15:31:32 -07:00
|
|
|
var table = JX.$H(response.changeset).getNode();
|
2012-01-05 20:29:16 -08:00
|
|
|
var root = context.parentNode;
|
Don't mangle inline comments with tables in them in Differential
Summary:
Fixes T3814. Broadly, remarkup tables in inline comments did not work properly. I ran into several messes here and cleaned up some of them:
- Some of this code is doing `JX.$N('div', {}, JX.$H(response.markup))`, to turn an HTML response into a node, passing that around, and then doing junk with it. This is super old and gross.
- The slightly more modern pattern is `JX.$H(response.markup).getFragment().firstChild`, but this is kind of yuck too and not as safe as it could be.
- Introduce `JX.$H(response.markup).getNode()`, which actually expresses intent here. We have a bunch of `getFragment().firstChild` callsites which should switch to this, but I didn't clean those up yet because I don't want to test them all.
- Switch the `JX.$N('div', {}, JX.$H(response.markup))`-style callsites to `JX.$H(response.markup).getNode()`.
- `copyRows()` is too aggressive in finding `<tr />` tags. This actually causes the bug in T3814. We only want to find these tags at top level, not all tags. Don't copy `<tr />` tags which belong to some deeper table.
- Once this is fixed, there's another bug with mousing over the cells in tables in inline comments. We select the nearest `<td />`, but that's the cell in the remarkup table. Instead, select the correct `<td />`.
- At this point, these last two callsites were looking ugly. I provided `findAbove()` to clean them up.
Test Plan: Created, edited, deleted, moused over, and reloaded a revision with inline comments including remarkup tables. Used "Show more context" links.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3814
Differential Revision: https://secure.phabricator.com/D6924
2013-09-10 15:31:32 -07:00
|
|
|
copyRows(root, table, context);
|
2012-01-05 20:29:16 -08:00
|
|
|
root.removeChild(context);
|
2011-01-31 20:38:13 -08:00
|
|
|
}
|
|
|
|
|
|
|
|
|
|
JX.Stratcom.listen(
|
|
|
|
|
'click',
|
|
|
|
|
'show-more',
|
|
|
|
|
function(e) {
|
2012-01-05 20:29:16 -08:00
|
|
|
var event_data = {
|
|
|
|
|
context : e.getNodes()['context-target'],
|
|
|
|
|
show : e.getNodes()['show-more']
|
|
|
|
|
};
|
|
|
|
|
|
|
|
|
|
JX.Stratcom.invoke('differential-reveal-context', null, event_data);
|
|
|
|
|
e.kill();
|
|
|
|
|
});
|
|
|
|
|
|
|
|
|
|
JX.Stratcom.listen(
|
|
|
|
|
'differential-reveal-context',
|
|
|
|
|
null,
|
|
|
|
|
function(e) {
|
|
|
|
|
var context = e.getData().context;
|
|
|
|
|
var data = JX.Stratcom.getData(e.getData().show);
|
|
|
|
|
|
2012-08-29 16:34:52 -07:00
|
|
|
var container = JX.DOM.scry(context, 'td')[0];
|
2011-01-31 20:38:13 -08:00
|
|
|
JX.DOM.setContent(container, 'Loading...');
|
|
|
|
|
JX.DOM.alterClass(context, 'differential-show-more-loading', true);
|
Fix whitespace and unchanged file shields
Summary:
Fixes T181. I actually have no idea what the original issue in T181 was, but this fixes several problems:
- The code for figuring out whitespace-only changes was extremely confusing and probably buggy (the code for figuring out unchanged files was equally confusing but probably less buggy).
- When rendering a whitespace shield, we did not offer a "Show Changes" link. Instead, show the "Show Changes" link: we can always render the content beneath this link.
- When clicking "Show Changes", we used the current whitespace mode, which might result in a diff with no changes. Instead, force "show all" whitespace mode.
- We never offered a "show changes" link for unchanged files. Instead, offer this link if we can service it.
- When clicking "Show Changes", we pierced the shield but didn't force file content, which would fold the entire file even if it was available. Instead, force the file content.
Test Plan: Generated whitespace-only and no-change diffs. Clicked shield links, or verified no shield link available in no-change-with-no-content. Generated an "@generated" change, verified shield worked correctly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T181, T2009
Differential Revision: https://secure.phabricator.com/D4407
2013-01-11 15:27:42 -08:00
|
|
|
|
|
|
|
|
if (!data['whitespace']) {
|
|
|
|
|
data['whitespace'] = config.whitespace;
|
|
|
|
|
}
|
|
|
|
|
|
2011-07-16 07:09:19 -07:00
|
|
|
new JX.Workflow(config.uri, data)
|
2012-01-05 20:29:16 -08:00
|
|
|
.setHandler(JX.bind(null, onresponse, context))
|
2011-07-16 07:09:19 -07:00
|
|
|
.start();
|
2011-01-31 20:38:13 -08:00
|
|
|
});
|
|
|
|
|
|
|
|
|
|
});
|
|
|
|
|
|
|
|
|
|
function copyRows(dst, src, before) {
|
|
|
|
|
var rows = JX.DOM.scry(src, 'tr');
|
|
|
|
|
for (var ii = 0; ii < rows.length; ii++) {
|
Don't mangle inline comments with tables in them in Differential
Summary:
Fixes T3814. Broadly, remarkup tables in inline comments did not work properly. I ran into several messes here and cleaned up some of them:
- Some of this code is doing `JX.$N('div', {}, JX.$H(response.markup))`, to turn an HTML response into a node, passing that around, and then doing junk with it. This is super old and gross.
- The slightly more modern pattern is `JX.$H(response.markup).getFragment().firstChild`, but this is kind of yuck too and not as safe as it could be.
- Introduce `JX.$H(response.markup).getNode()`, which actually expresses intent here. We have a bunch of `getFragment().firstChild` callsites which should switch to this, but I didn't clean those up yet because I don't want to test them all.
- Switch the `JX.$N('div', {}, JX.$H(response.markup))`-style callsites to `JX.$H(response.markup).getNode()`.
- `copyRows()` is too aggressive in finding `<tr />` tags. This actually causes the bug in T3814. We only want to find these tags at top level, not all tags. Don't copy `<tr />` tags which belong to some deeper table.
- Once this is fixed, there's another bug with mousing over the cells in tables in inline comments. We select the nearest `<td />`, but that's the cell in the remarkup table. Instead, select the correct `<td />`.
- At this point, these last two callsites were looking ugly. I provided `findAbove()` to clean them up.
Test Plan: Created, edited, deleted, moused over, and reloaded a revision with inline comments including remarkup tables. Used "Show more context" links.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3814
Differential Revision: https://secure.phabricator.com/D6924
2013-09-10 15:31:32 -07:00
|
|
|
|
|
|
|
|
// Find the table this <tr /> belongs to. If it's a sub-table, like a
|
|
|
|
|
// table in an inline comment, don't copy it.
|
|
|
|
|
if (JX.DOM.findAbove(rows[ii], 'table') !== src) {
|
|
|
|
|
continue;
|
|
|
|
|
}
|
|
|
|
|
|
2011-01-31 20:38:13 -08:00
|
|
|
if (before) {
|
|
|
|
|
dst.insertBefore(rows[ii], before);
|
|
|
|
|
} else {
|
|
|
|
|
dst.appendChild(rows[ii]);
|
|
|
|
|
}
|
|
|
|
|
}
|
Add "Undo" for editing Differential inline comments
Summary:
When a user hits 'cancel' on a 'new', 'edit', or 'reply' operation, add a little
"Changes discarded. __Undo__" insert so they can get their change back. No undo
for delete since there's an explicit prompt. Once this lands we can make
'escape' work again to close dialogs.
This change started feeling really good when I was merging all the duplicate
code and making things more consistent, but by the time I started writing client
rendering it felt gross. I'm not really thrilled with it but I guess it's a step
forward? The feature seems pretty OK in practice. Let me know how much barfing
this causes and I can try to remedy the most acute concerns.
This also fixes a bug where replies always (?) appear on the 'new' side of the
diff (I think?).
Test Plan:
Applied 'new', 'edit', 'delete' and 'reply' operations, pressed 'cancel' and
'okay' in each case, with and without changing text where relevant. All
behaviors seem to conform with expectations, except that canceling out of 'edit'
without changing the text gives you an option to undo when it shouldn't really.
There's no super easy way to get at the original text right now.
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen
CC: simpkins, aran, epriestley
Differential Revision: 406
2011-06-07 16:11:10 -07:00
|
|
|
return rows;
|
2011-01-31 20:38:13 -08:00
|
|
|
}
|