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

fix: card height in grid #1199

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from
Open

fix: card height in grid #1199

wants to merge 7 commits into from

Conversation

akloeckner
Copy link
Collaborator

@akloeckner akloeckner commented Jan 13, 2025

This is my take on the sections/grid issue. :-)

This changes the card height to occupy the assigned height in grid/sections view. The Graph will be stretched to fill this space! That means, its aspect ration will change. Change the height option to make it look better. See this example:
image

I consider this better than simply fixing the card height, because the card remains with lots of empty space in this case. If we fill the space, the default look will be nicer. Here's how it looks, if only the card height is fixed:
image

Here's how it is done technically:

  • :host.height = 100%
    => fixes height of entire card to fill space, if there is a defined space
  • graph.flex = auto
    => assigns remaining space in card to the graph itself
  • graph_container.height = 100%
    => makes the container fill the entire space
  • graph__container__svg:
    .position=relative
    .width=100%
    .height=100%
    => makes the container use the entire space. Not only in flex width.
  • The relative positioning changes the z-order of the graph and the labels. So, this is fixed in the main source code.
  • The SVG also is freed aspect-ratio-wise, otherwise it could not fill the entire space.

This changes the card height to occupy the assigned height in grid/sections view.
The Graph will be stretched to fill this space.
Its aspect ration will change. Change the `height` option to make it look better.

How is it done:

:host.height = 100%
=> fixes height of entire card to fill space, if there is a defined space

graph.flex = auto
=> assigns remaining space in card to the graph itself

graph_container.height = 100%
=> makes the container fill the entire space

graph__container__svg:
.position=relative
.width=100%
.height=100%
=> makes the container use the entire space. Not only in flex width.

The relative positioning changes the z-order of the graph and the labels.
So, this is fixed in the main source code.

The SVG also is freed aspect-ratio-wise, otherwise it could not fill the entire space.
src/style.js Outdated
@@ -4,6 +4,7 @@ const style = css`
:host {
display: flex;
flex-direction: column;
height: 100%;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check these examples:
image
1st card - default
2nd - added height: 100% for :host
3rd - my PR

type: horizontal-stack
title: stack
cards:
  - type: custom:mini-graph-card
    name: default
    entities:
      - entity: sensor.xiaomi_cg_1_co2
  - type: custom:mini-graph-card
    name: height:100% for :host
    entities:
      - entity: sensor.xiaomi_cg_1_co2
    card_mod:
      style: |
        :host {
          height: 100%;
        }
  - type: custom:mini-graph-card
    name: my PR
    entities:
      - entity: sensor.xiaomi_cg_1_co2
    card_mod:
      style: |
        :host {
          display: unset !important;
          flex-direction: unset !important;
        }
        ha-card {
          height: 100%;
        }
  - type: calendar
    entities:
      - calendar.calendar_1
type: grid
columns: 4
cards:
  - type: custom:mini-graph-card
    name: default
    entities:
      - entity: sensor.xiaomi_cg_1_co2
  - type: custom:mini-graph-card
    name: height:100% for :host
    entities:
      - entity: sensor.xiaomi_cg_1_co2
    card_mod:
      style: |
        :host {
          height: 100%;
        }
  - type: custom:mini-graph-card
    name: my PR
    entities:
      - entity: sensor.xiaomi_cg_1_co2
    card_mod:
      style: |
        :host {
          display: unset !important;
          flex-direction: unset !important;
        }
        ha-card {
          height: 100%;
        }
  - type: calendar
    entities:
      - calendar.calendar_1
title: grid

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the 100% on host were not enough. But maybe the first 100% needs to go where you put it in your PR... I'll check this. This is what a horizontal stack looks like with a scaled graph:
image

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks nice imho. Better than a tiny one. I will continue a review soon.
Also, we should check how a height option affects...

Copy link
Collaborator

@ildar170975 ildar170975 Jan 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: by default any card inside horiz stack gets display: block; inside grid - display: inline (automatically) - unless it is changed by a card's creator.

Copy link
Collaborator Author

@akloeckner akloeckner Jan 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The height option sets the graph height in masonry. But if the height is already fixed by the view (grid, sections, horizontal stack), then the height option only affects the squishing of the stroke.

Wherever we continue with this, we should have selvalt in the loop. They had some idea to counter the squishing and started the discussion on this feature. I am afraid, I diluted the discussion a bit now...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so we seem to agree on this. I also checked the latest changes in #1139. I like the looks of the graph very much. Very neat fix for the line and point styles, @selvalt7! I also saw, you picked up the latest changes from #1155.

I therefore cherry-picked your points rendering and fixed up some remnant styles on the go. I also cherry-picked @ildar170975's latest change to the height of :host vs. ha-card.

So, I think, we now have a minimal set of changes required to properly support sections view. And it will be attributed to all of us. ;-)

Could you please test this PR and see, if we're missing any edge cases? I did some testing and all seems to work fine, but you had a deeper dive into these issues than I had...

I suggest then, we finish this PR here and then go on to discuss the other functionality in #1139 and possibly add it step by step. OK?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have found one edge case, with sections, when graph clips outside the card with this config:

show:
  extrema: true
layout_options:
  grid_columns: 4
  grid_rows: 4

I came up with an idea to set min-height: 0 in this selector but also it could be done with size limiting in #1139.
Other than that, it seems fine to me.

Copy link
Collaborator Author

@akloeckner akloeckner Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. I played around a bit and there are lots of possibilities, how a user could make the card too small to fit everything in. If we make the card small enough, even the state will clip out of the bottom.

I think, this cannot and should not really be avoided automatically in the general case. At least YAML power users might have legitimate reasons to do this. Then, they'll have to live with the card being cut off. Even this might make sense for some:

layout_options:
  grid_columns: 2
  grid_rows: 2
height: 10
line_width: 1

However, the case you outlined should be fixed, because there is actually sufficient space available. The graph simply clips out of bound, because it has a default height set to 100px. And that is too tall. We should not change the default height, I think, because it would affect most of the users. We should not change it in section view only either, because this is only possible after initialization. So, I guess the CSS approach should be the way to go. 👍

I'll tried it and it works nicely!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably we should allow to set a height manually - but it should be limited: not more than a possible value.
Assume someone need to have a SMALLER graph? (Like for adding additional elements above a graph with card-mod)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I was tempted to move this discussion into the main thread, but I realized, we're discussing here how advanced users might use the card. So, I'll keep it separate, in order for us to be able to close the discussion, once we're agreed.)

Probably we should allow to set a height manually - but it should be limited: not more than a possible value.

That would mean, the documented behaviour would be:

  • Graph will have actual height of height setting divided by 500, times the actual graph width. (Which is bit of legacy stupidity already.)
  • In sections view (or any other location that fixes a total card height), the actual height of the graph will be limited to the available space.
  • If you want the graph to fill all the available space, set a suitable height setting well above the imaginable available space.

While I think, that should be possible by setting a flex option which doesn't allow growing (but only shrinking)... I think, this is quite un-intuitive... A "normal" (as opposed to "advanced") user would expect the graph to just look nice without setting an arbitrary but high enough height. Wouldn't they?

Assume someone need to have a SMALLER graph? (Like for adding additional elements above a graph with card-mod)

I don't think we should design the card to be as hackable as possible. At least not, if it impairs the "normal" user's experience. I think, if people have such specific needs for modifications, they will find a way, regardless of what we do... For example, they could simply override the flex setting themselves.

@akloeckner
Copy link
Collaborator Author

I noticed another glitch, which I haven't been able to fix yet: Setting an absurd height option on the card (e.g. height: 5000) makes parts of the graph be clipped. See the top of this graph:
image

I believe it is linked to the mask inside the SVG. I wouldn't consider it a show-stopper. But if you have an idea how to fix it, we could do it already...

@ildar170975
Copy link
Collaborator

ildar170975 commented Jan 15, 2025

What we have now:
The card is not properly placed in a cell in Grid card - does not occupy an available area.
This can be solved by a proper combination of display & height properties (like it was done already in all three PRs).

Next - the graph's DEFAULT height should not cause a "tiny graph in a high card" case.
This is what discussed in this current PR of @akloeckner.

Next - margins/paddings/fonts etc should adapt to a card's size.
This was discussed in PR of @selvalt7.

Guys, lets solve the task step-by-step:

  1. Solve the minimal task about a card's height.
  2. The graph may be tiny (if a card is "stretched up" vertically to adapt to a higher cell) - but a user may fix it by a proper height. Means - the default look may be bad, but a user can fix it.
  3. Next - we add a nicely set default graph's height.
  4. Next - we add a further customization if required.

I.e. - let's issue a release with at least 1 solved task.

README.md Outdated Show resolved Hide resolved
@akloeckner
Copy link
Collaborator Author

  1. The graph may be tiny (if a card is "stretched up" vertically to adapt to a higher cell) - but a user may fix it by a proper height. Means - the default look may be bad, but a user can fix it.

I don't think, we should release something like this. Because users will change the heights of their graphs. And then we come and change the card's behaviour, again! If we know, it is not desirable, we should avoid it. That's why I would prefer to solve this step together with the first step. That way, we provide something which works out of the box for the "normal" users. If then some power users asks for an interface to hack the card, we can consider it.

Next - margins/paddings/fonts etc should adapt to a card's size.

This should be discussed later, I agree.

To be honest: I think, the solution we merged together from our three branches here is basically the solution to your steps 1, 2 and 3 already. Without an "ugly" intermediate step. Unless I am missing something (other than the open discussion on the card-mod hackability). So, I would suggest to take these three steps. And then think about the next. If you're concerned about traceability, we can merge your PR first and base this one on yours. But I would not create an intermediate release.

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

Successfully merging this pull request may close these issues.

3 participants