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 7 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
7 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
6753019
Alternate points rendering
selvalt7 Jan 3, 2025
3aabe59
Update style.js
ildar170975 Dec 26, 2024
6a56bf8
fix: graph clips out of bottom in small cards
akloeckner Jan 14, 2025
6d74e07
doc: height disregarded in sections view
akloeckner Jan 15, 2025
531864b
doc: improve grid description
akloeckner Jan 15, 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 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?

Copy link

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

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.

}
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