Skip to content

Truncate _source summary in doc table #9394

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Bargs wants to merge 2 commits into from
Closed

Truncate _source summary in doc table #9394

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9537a0b
Truncate _source summary in doc table
Bargs Dec 6, 2016
4717531
Merge branch 'master' into truncateSource
spalger Dec 13, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions src/ui/public/doc_table/components/table_row.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,11 @@ module.directive('kbnTableRow', function ($compile) {
});

const $target = reuse ? $(reuse).detach() : $(html);

if ($target.hasClass('discover-table-sourcefield')) {
truncateSourceSummary($target);
}

$target.data('discover:html', html);
const $before = $cells.eq(i - 1);
if ($before.size()) {
Expand Down Expand Up @@ -158,6 +163,22 @@ module.directive('kbnTableRow', function ($compile) {

return text;
}

function truncateSourceSummary(sourceElement) {
Copy link
Copy Markdown
Contributor

@lukasolson lukasolson Dec 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to me it might make more sense to have this logic here. Then you don't have to check if the "discover-table-sourcefield" class exists, nor deal with the DOM. What do you think?

Copy link
Copy Markdown
Contributor Author

@Bargs Bargs Dec 9, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about that, went back and forth a bit and I could still be convinced but here was my thinking:

The source formatter is a generic utility that could be used in lots of places, so while we want a truncated version in the doc table we might not want a truncated version everywhere the source formatter is used. Since it's the doc table that has specific requirements for the content, it's best suited to do the truncation.

But I just realized the current implementation won't work. Someone could theoretically specify a different field formatter for the _source field, and this logic would break because it expects a particular html structure.

I thought about defining a new content type, so in addition to text and html maybe we'd have something like html-summary that the doc table could request but I dunno, it feels a little hacky, html-summary isn't really a content type. Do you have any ideas? I'm not super familiar with the field formatter code

const sourceList = sourceElement.find('dl');
const listItems = sourceList.children();
const listPairs = _.chunk(listItems, 2);
sourceList.empty();

let length = 0;
_.forEach(listPairs, (listPair) => {
length += (listPair[0].textContent.length + listPair[1].textContent.length);
if (length < 500) {
sourceList.append(listPair);
sourceList.append(' ');
}
});
Copy link
Copy Markdown
Member

@weltenwort weltenwort Dec 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still performs a lot of individual DOM manipulations. What about first calculating the number of elements to keep and then removing any additional children of sourceList in one operation (e.g. using the :gt(index) selector and .remove())?

Copy link
Copy Markdown
Contributor Author

@Bargs Bargs Dec 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since sourceList hasn't been injected into the document yet, is appending really an expensive operation? The browser won't need to calculate style, layout, etc until the template is compiled here, right?

Copy link
Copy Markdown
Member

@weltenwort weltenwort Dec 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're correct. 😊 Still, since you're optimizing for performance just pushing individual items onto the collection might be costly. Probably depends on how this is implemented internally in jQuery/jqLite.

}
}
};
});