[Lens] Table column text alignment#89300
Conversation
|
Pinging @elastic/kibana-app (Team:KibanaApp) |
dej611
left a comment
dej611
left a comment
There was a problem hiding this comment.
All ok, but one tricky case with Unique count.
Tested in Firefox, Chrome and Safari. FF is all good, but the other two Webkit-based browsers show the scrollbar issue described above
| 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 |
mbondyra
left a comment
mbondyra
left a comment
There was a problem hiding this comment.
Tested in Safari, apart from the two issues mentioned in this PR, all works as expected. LGTM 🙆♀️
wylieconlon
left a comment
wylieconlon
left a comment
There was a problem hiding this comment.
I reviewed and found that overall this is working well, but I did find 2 areas beyond what you've previously commented on that I think we should discuss before approving.
| 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: |
wylieconlon
left a comment
wylieconlon
left a comment
There was a problem hiding this comment.
Changes LGTM, tested yesterday without committing
8 participants
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.