Skip to content
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.

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

Preserve whitespace in discover table cells for added fields #4102

Merged
merged 6 commits into from
Jun 4, 2015

Conversation

ycombinator
Copy link
Contributor

Fixes #3993.

@rashidkpc rashidkpc self-assigned this Jun 3, 2015
<td <%= timefield ? 'class="discover-table-timefield" width="1%"' : '' %>><%= formatted %></td>
Copy link
Contributor

Choose a reason for hiding this comment

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

Pretty sure there are other templates that need to have the whitespace stripped from them (like src/kibana/components/doc_table/components/table_row/details.html). Consider using the no_white_space utility to accomplish this rather than turning them all into single-line templates.

@rashidkpc rashidkpc assigned spalger and unassigned rashidkpc Jun 3, 2015
@rashidkpc rashidkpc assigned ycombinator and unassigned spalger Jun 3, 2015
@ycombinator ycombinator assigned spalger and unassigned ycombinator Jun 4, 2015
@ycombinator
Copy link
Contributor Author

@spalger I've used the no_white_space utility as you suggested but I'm not terribly happy with how I had to use it in two places - first to remove whitespace from the <td> element and then to remove whitespace from any HTML within it (e.g. the <div class="truncate-by-height"> element). Let me know if you think of a better way of doing this.

@@ -104,10 +105,10 @@ define(function (require) {
}

$scope.columns.forEach(function (column) {
newHtmls.push(cellTemplate({
newHtmls.push(noWhiteSpace(cellTemplate({
Copy link
Contributor

Choose a reason for hiding this comment

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

You shouldn't need the noWhiteSpace filter here. The cellTemplate was already stripped of whitespace so any that shows up in the results is from the value and should be shown.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing the noWhiteSpace filter here causes this display for the _source column:
screen shot 2015-06-04 at 1 19 35 pm

Adding the filter back fixes the display:
screen shot 2015-06-04 at 1 23 13 pm

I too think its odd that I need to apply this filter over here. I'm guessing there's another way to fix the _source display issue (when the filter is removed) but I haven't been able to figure out how.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say that the _source field shouldn't have significant white-space. It if does then a couple new lines will hide the rest of the fields. What do you think @rashidkpc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I found a better fix. It still involves using noWhiteSpace in two places but I think its the right way to use it. Please hold while I commit.

var cellTemplate = _.template(require('text!components/doc_table/components/table_row/cell.html'));
var truncateByHeightTemplate = _.template(require('text!partials/truncate_by_height.html'));
var cellTemplate = _.template(noWhiteSpace(require('text!components/doc_table/components/table_row/cell.html')));
var truncateByHeightTemplate = _.template(noWhiteSpace(require('text!partials/truncate_by_height.html')));
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

@spalger
Copy link
Contributor

spalger commented Jun 4, 2015

LGTM!

spalger added a commit that referenced this pull request Jun 4, 2015
Preserve whitespace in discover table cells for added fields
@spalger spalger merged commit dac6df7 into elastic:master Jun 4, 2015
@ycombinator ycombinator deleted the gh-3993 branch February 20, 2016 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants