search

vaadin / flow-components

Java counterpart of Vaadin Web Components
101 stars 66 forks source link

Grid.Column implements HasStyle but it doesn't seem to work #6106

Open archiecobbs opened 7 months ago

archiecobbs commented 7 months ago

Description

I have a Grid.Column and I say getStyle().set("font", "Courier") but it doesn't change the font of what's displayed.

Expected outcome

Fixed-width font for my column data.

Minimal reproducible example

I'm not a CSS or shadow DOM expert. But what I see is:

Doing this: column.getStyle().set("font", "Courier")

Results in this:

Screenshot 2024-03-07 at 12 09 56 PM

Steps to reproduce

  1. Set column.getStyle().set("font", "Courier")
  2. Display Grid
  3. Notice font in column is not Courier

Environment

Vaadin version(s): 24.3.5 OS: Mac OS

Browsers

Firefox

Legioth commented 7 months ago

This is a side effect from the fact that Column extends Component and we have opted to make Component implement HasStyle since 99% of all components do support styles in a sensible way. Column is one of the exceptions. To apply styles to the cells in a column, you can use Column::setClassNameGenerator or Column::setPartNameGenerator in cases when the styles can be defined in a CSS file. For other cases, you would have to define a custom renderer.

I can see multiple alternatives for addressing this issue, but they all have tradeoffs:

  1. Make Grid apply column styles to all its cells. Might lead to weird situations and surprises.
  2. Override the getStyle() method in Column only to mark it as deprecated. Easy but limited.
  3. Refactor Column to not be a Component. Relatively big effort and probably a breaking change in some cases.
  4. Remove HasStyle from Component. Semantically correct but big breaking change.
archiecobbs commented 7 months ago

I can see multiple alternatives for addressing this issue, but they all have tradeoffs:

My vote is for #1. My reasoning is that it's likely that very few people are currently using Grid.Column.getStyle() because it doesn't do anything. So the change should be 99% backward compatible. Moreover adding this support imposes no cost to anyone who doesn't use it (or for any columns that don't have their style modified), so we're not breaking anything that already exists.

Not sure what "surprises" one might find other than something that didn't work before suddenly starts working :)

My second choice would be to vote for adding documentation to the Grid.Column API Javadoc explaining that the HasStyle methods don't work for the class, so other people don't waste their time falling into this same trap.

Thanks.