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.

Sign up for GitHub

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 2 commits into
base: dev
Choose a base branch
from
Open

fix: card height in grid #1199

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f96100b
fix: card height in grid
akloeckner Jan 13, 2025
3848307
simple fix for linestroke preserving aspect ratio
akloeckner Jan 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,11 +329,11 @@ class MiniGraphCard extends LitElement {
<div class="graph">
${ready ? html`
<div class="graph__container">
${this.renderLabels()}
${this.renderLabelsSecondary()}
<div class="graph__container__svg">
${this.renderSvg()}
</div>
${this.renderLabels()}
${this.renderLabelsSecondary()}
</div>
${this.renderLegend()}
` : html`
Expand Down Expand Up @@ -539,6 +539,7 @@ class MiniGraphCard extends LitElement {
const { height } = this.config;
return svg`
<svg width='100%' height=${height !== 0 ? '100%' : 0} viewBox='0 0 500 ${height}'
preserveAspectRatio='none'
@click=${e => e.stopPropagation()}>
<g>
<defs>
Expand Down
7 changes: 6 additions & 1 deletion src/style.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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 •
edited
Loading

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

Choose a reason for hiding this comment

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

this does not make our life much easier

The thing is that we should not change display like it was done long time ago (that time grid card was not invented yet).
To keep a proper behaviour - we need to use default values (i.e. not overwrite them).

Copy link
Collaborator

Choose a reason for hiding this comment

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

What I see is
image
because of preserveAspectRatio='none'.
SVG is elongated.
We need to think more...

Copy link
Collaborator

@ildar170975 ildar170975 Jan 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We probably need to set this "0 0 500 100" thing dynamically dependently on an available height:
image

Copy link
Collaborator

@ildar170975 ildar170975 Jan 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Note that if the height option is set (to 1000 for example) - it is accounted in that SVG size:
image
Means - we may LEAVE to a user to define how the card will look if stretched))))
Anyway, assume we found a way to stretch a graph properly. Then a user comes and asks "WTH 'height' option is not accounted?"
The funniest part: seems that in this case your PR will be same as my PR ))) and it will be 3rd concurrent PR with same code for "height")

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(Tagging @selvalt7 to have them in this discussion on the desired looks of the stretched card in grid/sections view.)

because of preserveAspectRatio='none'.
SVG is elongated.

Yes. It can be fixed for the lines, as selvalt pointed out. Points are more difficult. But there is a suggestion in #1139, at which I couldn't look closer yet.

We probably need to set this "0 0 500 100" thing dynamically

That is what selvalt proposed. But it recalculates the entire graph each time the window is resized. (Basically, because the number of rows is only available after the Graph is initialized.) I think it will be more natural and efficient to leave such resizing to CSS.

we may LEAVE to a user to define how the card will look if stretched

That might make sense, actually... It would also make the change much easier. It will just not look pretty by default. :-/

WTH 'height' option is not accounted?

It is not accounted correctly currently either: the width is set to 500 and the height to whatever is in the option. Then, the graph is scaled (preserving aspect ratio) to fit the card's width. This changes the height. By the way, this also changes line widths, which can be set in the options, but are thus not respected correctly either. So, these dimensions are a sensible first guess, in the best case. And completely useless in the worst case. :-P Based on this observation, I would even consider to remove this option... Then again, there would be no way to set a height in default masonry view.

seems that in this case your PR will be same as my PR ))

I have no problem to close mine. I just wanted to create something focused and testable to discuss the stretched approach.

In summary, I think, we should

  • for default masonry view
    • at least have a way to set height in default view, so the option must stay.
    • But maybe we should treat the height option differently: use it as actual height and only scale the graph in width.
    • This would require some de-squishing of the graph anyway. Which would, as a side-effect, ideally make the line width match what is set by the user.
  • For sections view,
    • we should disregard the height option and use available space. Because we have another height option (the number of rows) to respect, there is no need to respect contradicting options.
    • Alternatively, if this de-squishing becomes too hard, we should leave the height of the graph in section view up to the user and accept an initial ugly space in the card...
    • We should decide on the way to handle height in sections before we introduce the support for them. Otherwise people will start using it and the later fix becomes actually a breaking change...

So, I think, the questions are:

  1. Do we prefer to scale graphs in section view? From a simplicity perspective, I would say: yes. And I take @ildar170975's comment and @selvalt7's PR as a yes, too.
  2. Do we succeed to make that happen? If we cannot, we have to fall back to the manual height setting.

I will thus have a look at the de-squishing of points @selvalt7 proposed and see how far I get with question no. 2.

But, please, let me know, if you think differently on the preferred way of sizing the card! That could save me some work. ;-)

}
ha-card {
flex-direction: column;
Expand Down Expand Up @@ -234,6 +235,7 @@ const style = css`
right: 0;
}
.graph {
flex: auto;
align-self: flex-end;
box-sizing: border-box;
display: flex;
Expand All @@ -245,10 +247,13 @@ const style = css`
display: flex;
flex-direction: row;
position: relative;
height: 100%;
}
.graph__container__svg {
cursor: default;
flex: 1;
position: relative;
width: 100%;
height: 100%;
}
svg {
overflow: hidden;
Expand Down
Loading