Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Setting the Row height looses centering of the text #7973

Closed
pfteter opened this issue May 15, 2024 · 4 comments
Closed

Setting the Row height looses centering of the text #7973

pfteter opened this issue May 15, 2024 · 4 comments
Labels

Comments

@pfteter
Copy link

pfteter commented May 15, 2024

I'm submitting a ... ```
[X] bug report => see 'Providing a Reproducible Scenario'
[] feature request => do not use Github for feature requests, see 'Customers of AG Grid'
[] support request => see 'Requesting Community Support'


**Current behavior**
https://www.ag-grid.com/angular-data-grid/row-height/#getrowheight-callback

all text in cells should be centered. see url / image. Because of line-height not being adjusted after setting the height the text is not centered


**Expected behavior**

The text should be centered  - it's done with line-height calculation but when getrowheight is specified the linheight is not adjusted so the cell height is 200px but the line-height that is usually used to center it is just 40px so it's not possible to center content when row-height is set without a custom cell renderer

should be possible to center the text if the row height is set


using  https://www.ag-grid.com/javascript-data-grid/global-style-customisation-compactness/ 

could work in some cases but it's not doing the same

**Please tell us about your environment:**


* **AG Grid version:** X.X.X
latest

* **Browser:**
chrome


![image](https://github.com/ag-grid/ag-grid/assets/167297126/25e9c95e-31e3-4771-a567-73d99fb7d92d)


@BernieSumption
Copy link
Contributor

You can vertically align text in cells using CSS:

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

Here's a code sample (CSS has been added at the bottom of index.html): https://plnkr.co/edit/kQbNq6riZfKUazy3?open=app.component.ts&preview

@pfteter
Copy link
Author

pfteter commented May 21, 2024

That breaks text-overflow: ellipsis it's not acceptable since display: flex cannot be used with text-overflow ellipsis without the ellipsis content be wrapped into a container with for example display: block.

I guess this is also the reason ag grid uses line-height with calculations to center the content so that it still supports ellipsis.

So basically the default requested behaviour from the business/ multiple teams is to use ellipsis.

I still think this is a bug, it should use CSS vars to calculate the line-height correctly.

For all sizes line-height is uses to center the content and there is a good reason for it - ellipsis.
For getRowHeight its forced breaking the line-height calculation resulting in the content displayed on top.

There is a solution with a custom renderer that makes it complicated with multiple renderers or a special case where we for example activate autoHeight on the cell and ag grid will automatically add a wrapper in the cell (this is the workaround we are using now but that is a workaroud! ).

@AG-Zoheil
Copy link

Hello @pfteter,

Thank you for the suggestion.

Currently there's no way to implement this behaviour with AG Grid. Whilst we do not agree that this is a bug, we agree this would be a valuable addition to the product and we have added a feature request for this.

We have added this requirement to our backlog and we are tracking it with the following reference: AG-11667

We have no timeline for this at the moment, but this can still be picked up by our team when they're making modifications to this functional area of AG Grid. Please use the functionality currently provided by AG Grid until this is delivered at a later point.

See whether this item will be in the next release by checking the NEXT RELEASE checkbox on the product pipeline page:
https://www.ag-grid.com/ag-grid-pipeline/

The best way to track this is to sign up for AG Grid new release notifications using the instructions here. This way you'll know as soon as a new version is out and you can check whether this specific item was implemented on the changelog page.

Thanks again for bringing this up with us.

Kind regards,
Zoheil

@BernieSumption
Copy link
Contributor

I still think this is a bug, it should use CSS vars to calculate the line-height correctly.

Just to comment on this, the current behaviour is by design - the intention is that text in default height rows has the right amount of padding to make it appear vertically centred, but that if the row height becomes higher the text content is top aligned. Some applications rely on this behaviour to keep mixed-height content aligned. I agree it would be useful to have a feature to change the vertical alignment, I'm just explaining why this has to go through the feature request process rather than the bug fix process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants