[Lens] Table column text alignment #89300
Conversation
|
Pinging @elastic/kibana-app (Team:KibanaApp) |
| const isNumeric = | ||
| firstLocalTable.columns.find((dataColumn) => dataColumn.id === column.columnId)?.meta | ||
| .type === 'number'; | ||
| alignmentMap[column.columnId] = isNumeric ? 'right' : 'left'; | ||
| } |
There was a problem hiding this comment.
This logic here seems to power the following behaviour:
As long as the user didn't specify an alignment, it is right aligned for numbers, left aligned for the rest
The only issue I found is that it tries to guess the type from the "field" rather than the operation. An example of the issue can be seen with Unique count as column of any field:
Probably it's not a blocker as Unique count is the only operation affected (and of course any fullReference operation that uses Unique count as sub-operation).
|
Good catch! Previously I would look up the type in the datasource table spec, but that one is wrong in case of custom intervals (it thinks they are numbers). I will change it back. |
|
@dej611 Looking further into it, I think it's just wrong the unique count metric is setting |
|
@elasticmachine merge upstream |
| textAlign: currentAlignment, | ||
| }, | ||
| }); | ||
| }, [currentAlignment, setCellProps]); |
There was a problem hiding this comment.
The use of useEffect here is causing visible flickering during scrolls. I know that the docs recommend setCellProps, but why aren't we using class names? I did a local test using classnames and found that the flickering goes away.
I recorded a video of the flickering effect, and here is a single frame from the video:
As you can see in this frame, the text is not evenly aligned during the scroll.
There was a problem hiding this comment.
@flash1293 It looks like you're now assigning the alignment twice: there's a className and a useEffect. Do we need both?
There was a problem hiding this comment.
@wylieconlon eh, I forgot to remove the setCellProps hook. I will fix it tomorrow morning.
| frame.activeData[state.layerId].columns.find((col) => col.id === accessor)?.meta.type === | ||
| 'number' | ||
| ? 'right' | ||
| : 'left'); |
There was a problem hiding this comment.
Are we sure that we should be applying right alignment to numbers without guaranteed decimal places? For example, when looking at Average of bytes I think the defaults got worse:
There was a problem hiding this comment.
I don't think the default alignment should depend on whether or not decimal places are shown. I checked Google sheets and the Numbers app on Mac and both of them right align numbers without decimal places by default.
I think it's subjective whether the alignment looks better or worse - consistency is more important here IMHO. Once the bug with the horizontal scrollbar is gone it will be better as well I think.
|
@wylieconlon I switched it to using a classname, let me know whether it looks better for you now. |
💚 Build SucceededMetrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
Fixes #14524
Adds an option to set the text alignment of a column (left/right/center)
Behavior
Data grid has a bug with vertical scroll bars which becomes super obvious when implementing this feature. elastic/eui#4468 should take care of this, but this PR shouldn't be merged if it's not clear the EUI fix will be in the same version.