Skip to content
This repository has been archived by the owner on Nov 6, 2019. It is now read-only.

Handle cell text overflow with ellipsis or clipping #341

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Handle cell text overflow with ellipsis or clipping #341

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
181dfb9
Configure text renderer's overflow behavior
nmichaud May 31, 2018
b167d4e
Add configurability of cell text overflow behavior
nmichaud May 31, 2018
b10415f
Use clipping as the default overflow mode
nmichaud Jun 7, 2018
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
46 changes: 44 additions & 2 deletions packages/datagrid/src/textrenderer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ class TextRenderer extends CellRenderer {
this.backgroundColor = options.backgroundColor || '';
this.verticalAlignment = options.verticalAlignment || 'center';
this.horizontalAlignment = options.horizontalAlignment || 'left';
this.overflow = options.overflow || 'ellipsis';
Copy link
Member

Choose a reason for hiding this comment

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

Should the default be 'clip'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From an efficiency/cost perspective, it should be clip, but from a default UX experience I feel ellipsis are more user friendly. What do you think?

Copy link
Member

@sccolbert sccolbert Jun 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I was going based on what line 334 says.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah sorry, I was saying that either value as default makes sense, depending
on whether you want to optimize the speed or the UX.

this.format = options.format || TextRenderer.formatGeneric();
}

Expand Down Expand Up @@ -59,6 +60,11 @@ class TextRenderer extends CellRenderer {
*/
readonly horizontalAlignment: CellRenderer.ConfigOption<TextRenderer.HorizontalAlignment>;

/**
* The overflow behavior for the cell text.
*/
readonly overflow: CellRenderer.ConfigOption<TextRenderer.Overflow>;

/**
* The format function for the cell value.
*/
Expand Down Expand Up @@ -186,6 +192,7 @@ class TextRenderer extends CellRenderer {
// Set up the text position variables.
let textX: number;
let textY: number;
let boxWidth: number;

// Compute the Y position for the text.
switch (vAlign) {
Expand All @@ -205,13 +212,16 @@ class TextRenderer extends CellRenderer {
// Compute the X position for the text.
switch (hAlign) {
case 'left':
textX = config.x + 2;
textX = config.x + 8;
Copy link
Member

Choose a reason for hiding this comment

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

Why the change to the original padding? Should we make that configurable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably should be configurable. I found a slightly larger padding led to better white space balancing and made the grid much easier to scan and read when using it with real data.

Copy link
Member

Choose a reason for hiding this comment

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

IIRC I got the original padding values from Excel on Windows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll change it back, or add it as an option.

boxWidth = config.width - 14;
break;
case 'center':
textX = config.x + config.width / 2;
boxWidth = config.width;
break;
case 'right':
textX = config.x + config.width - 3;
textX = config.x + config.width - 8;
boxWidth = config.width - 14;
break;
default:
throw 'unreachable';
Expand All @@ -224,6 +234,25 @@ class TextRenderer extends CellRenderer {
gc.clip();
}

let overflow = CellRenderer.resolveOption(this.overflow, config);

if (overflow === 'ellipsis') {
let elide = '\u2026';
let textWidth = gc.measureText(text).width;

// Compute elided text
while ((textWidth > boxWidth) && (text.length > 1)) {
if (text.length > 4 && textWidth >= 2 * boxWidth) {
// If text width is substantially bigger, take half the string
text = text.substr(0, (text.length / 2) + 1) + elide;
} else {
// Otherwise incrementally remove the last character
text = text.substr(0, text.length - 2) + elide;
}
textWidth = gc.measureText(text).width;
}
}

// Set the gc state.
gc.font = font;
gc.fillStyle = color;
Expand Down Expand Up @@ -253,6 +282,12 @@ namespace TextRenderer {
export
type HorizontalAlignment = 'left' | 'center' | 'right';

/**
* A type alias for the supported overflow modes.
*/
export
type Overflow = 'clip' | 'ellipsis';

/**
* An options object for initializing a text renderer.
*/
Expand Down Expand Up @@ -293,6 +328,13 @@ namespace TextRenderer {
*/
horizontalAlignment?: CellRenderer.ConfigOption<HorizontalAlignment>;

/**
* The overflow behavior for text.
*
* The default is `'clip'`.
*/
overflow?: CellRenderer.ConfigOption<Overflow>;

/**
* The format function for the renderer.
*
Expand Down