Grid cell contents not vertically centered

[amcclain]

@johnbc and I were looking @ this in the context of a client app, but you can see these vertical misaligments in Toolbox as well.

We were looking in tiny mode (which is what we need to use for the app based on the client's requirements), and my sense is that the misalignment is more noticeable / distracting in tiny mode as they are larger in percentage-terms relative to the overall height.

Here is a close-up of a cell from Toolbox desktop sample grid in tiny mode, where the apparent top spacing is 2px greater than bottom:

and here is one of the mobile sample grid, where the offset is a bit worse, but biased towards too much space on the bottom:

The exact top/bottom boundaries of the fonts themselves are difficult to determine w/anti-aliasing and descenders, but I was looking to measure from the most visible / clear strong top/bottom boundaries of the primary glyphs.

Although these are small shifts, they have a noticeable and negative impact on the overall look of these grids, with some border-related configurations looking worse than others.

[TomTirapani]

We're using different line-height vars for each sizing mode to center the text - I can tweak the values, but before that, can anybody recall why we're not using flex for vertical alignment?

.ag-cell-value {
    display: flex;
    align-items: center;
}

I tried the above and it worked like a charm, and also allows us to remove the line-height vars entirely. Presumably there was some reason not to use flex for this?

[lbwexler]

Thanks tom -- sorry for the delay in responding! Need to get my act together with watching tickets.

@amcclain -- any thoughts? Seems like this will be much less fragile. toolbox and hoist-react ought to provide pretty good coverage for the different types of cells we render.

[amcclain]

I don't know of any special decision here, other than "not doing something that ag-grid is not doing" as a default.

Is it generally considered OK to use flex in this way - i.e. as a tool for vertical alignment? Is there any way or sense that this could be a performance issue of any kind? I have no idea - I am just responding to the idea of flex being some more complex or "new" layout system overall. Would/could it throw off component-based renderers?

I am not at all opposed to it, I just don't personally have any intuitive sense as to how much of a change or stake in the ground such a style rule represents (sorry - flex still something that regularly trips me up personally).

[TomTirapani]

Will try this next week and check throughout toolbox for regressions. I think the only risk is with custom elementRenderers, although I expect in most cases we'd want these vertically aligned.

[TomTirapani]

I gave this approach a try: https://github.com/xh/hoist-react/pull/2107

Checked throughout toolbox and admin, and all looked the grids / tree grids / renderers looked good to me

[lbwexler]

Thanks Tom!

[TomTirapani]

I encountered a regression with the above change, where our text-overflow ellipsis was not working for cells with html in their renderers (e.g. colored ledger values).

Addressed in the following commit: https://github.com/xh/hoist-react/commit/d07f5f4cd30d05196961534b40bc9d8f95340f7a