-
Notifications
You must be signed in to change notification settings - Fork 168
Handle cell text overflow with ellipsis or clipping #341
base: master
Are you sure you want to change the base?
Conversation
@@ -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'; |
There was a problem hiding this comment.
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'
?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@@ -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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Thanks for this! I made a few comments, but it looks really good as a whole. |
This provides an option for the default TextRenderer to show ellipses instead of clipping the text at the cell boundary.