Skip to content

fix: height in grid#1155

Open
ildar170975 wants to merge 3 commits intokalkih:devfrom
ildar170975:height-in-grid
Open

fix: height in grid#1155
ildar170975 wants to merge 3 commits intokalkih:devfrom
ildar170975:height-in-grid

Conversation

@ildar170975
Copy link
Copy Markdown
Collaborator

@ildar170975 ildar170975 commented Oct 2, 2024

Before:
image

  - type: grid
    columns: 2
    square: false
    title: not square
    cards:
      - &ref_graph_card
        type: custom:mini-graph-card
        entities:
          - entity: sensor.xiaomi_cg_1_co2
      - &ref_entity_card
        type: entity
        entity: sun.sun
      - *ref_entity_card
      - *ref_graph_card

  - type: grid
    columns: 2
    square: true
    title: square
    cards:
      - *ref_graph_card
      - *ref_entity_card
      - *ref_entity_card
      - *ref_graph_card

After:
image

Probably may be useful in case of using a new experimental "sections" views (honestly, do not care about this still beta feature).

@akloeckner akloeckner changed the title fix height in Grid fix: height in grid Oct 2, 2024
@akloeckner
Copy link
Copy Markdown
Collaborator

akloeckner commented Oct 2, 2024

Should we then merge this after #1139 ?

(honestly, do not care about this still beta feature)

What do you mean by this? Are sections still beta?

@ildar170975
Copy link
Copy Markdown
Collaborator Author

ildar170975 commented Oct 2, 2024

Are sections still beta?

Yes, still experimental... But many beginners use it and some of them seems like unaware about the old-style working way...

@ildar170975
Copy link
Copy Markdown
Collaborator Author

ildar170975 commented Oct 2, 2024 •
edited
Loading

Should we then merge this after #1139 ?

Recently got home, have not seen this PR yet.

@ildar170975
Copy link
Copy Markdown
Collaborator Author

ildar170975 commented Oct 2, 2024

Based on my estimation of users using sections: I would say that my PR may come first )))))

@selvalt7 selvalt7 mentioned this pull request Oct 5, 2024
Open
@Nickduino

This comment was marked as off-topic.

Sign in to view
@ildar170975

This comment was marked as off-topic.

Sign in to view
@akloeckner
Copy link
Copy Markdown
Collaborator

akloeckner commented Jan 13, 2025

I checked this out and, yes, it fixes the height of the card to the required grid rows... But: The graph remains very miniature and squashed to the bottom:

image

That is not what I would expect. I would expect the graph to fill the space defined by the number of rows...

I created #1199, which has my current working state based on the original sections PR. But I stripped out all the advanced features for the time being. Could you check this out and let me know, if this feels more intuitive to you, too? (It does for me.)

@Nickduino

This comment was marked as spam.

Sign in to view
@akloeckner akloeckner mentioned this pull request Jan 13, 2025
Open
@Liquidmasl
Copy link
Copy Markdown

Liquidmasl commented Apr 11, 2026

This indeed fixed the card for grids, but ignores the section view, which is now the new default for HA.

#1199 includes this fix, and also handles sections, while missing small things
#1139 also includes this and fixes sections, istnt really missing stuff, but hasnt it wired in correctly, and has slightly messier code.

Anyway, I would close this one, and concentrate on sections. Its the new default and it fixes grids in the same throw.

@ildar170975
Copy link
Copy Markdown
Collaborator Author

ildar170975 commented Apr 11, 2026

This indeed fixed the card for grids, but ignores the section view, which is now the new default for HA.

How it does ignore sections if it works in grid? Please elaborate your observation.

@Liquidmasl
Copy link
Copy Markdown

Liquidmasl commented Apr 11, 2026

This indeed fixed the card for grids, but ignores the section view, which is now the new default for HA.

How it does ignore sections if it works in grid? Please elaborate your observation.

sections have a bit more logic in how size is defined or fetched from HA. The whole section layouting.
If you check the other PRs you see the additional work that has gone into supporting sections.

This PR here just fixes size for grids, but not sections.

@ildar170975
Copy link
Copy Markdown
Collaborator Author

ildar170975 commented Apr 11, 2026

sections have a bit more logic in how size is defined or fetched from HA. The whole section layouting.

Any card supports default layout options.
What we can add here is just minimal number of columns/rows, fixed rows etc.
This PR adds a minimal support of stack-like cards - an absolute minimum for section support (not to mention horizontal stack and grid card).
A real difference between this PR and other ones - a support of dynamically changed graph height. This is CURRENTLY reviewed by me.

@Liquidmasl
Copy link
Copy Markdown

Liquidmasl commented Apr 11, 2026

sections have a bit more logic in how size is defined or fetched from HA. The whole section layouting.

Any card supports default layout options. What we can add here is just minimal number of columns/rows, fixed rows etc. This PR adds a minimal support of stack-like cards - an absolute minimum for section support (not to mention horizontal stack and grid card). A real difference between this PR and other ones - a support of dynamically changed graph height. This is CURRENTLY reviewed by me.

You are not wrong. But its redundant. 3 PRs just clog the pipeline that can be consolidated.
Merging this will just lead to additional work on the other, better PRs cause of potential conflicts.
This PR is 2 years old, and can just be closed in favor of #1199 .
#1139 can also be closed in favor of #1199 .
If there is already to little capacity for the repo, closing and consolidating PRs and issues WILL help with that.

@Liquidmasl
Copy link
Copy Markdown

Liquidmasl commented Apr 11, 2026 •
edited
Loading

dont get me wrong, but these changes are all extremely minimal, and reviewed in about 10 minutes, and then another 10 minutes for manual testing. And those are REAL bugs.
This PR is 2 YEARS old. I am confused. These fixes could have been shipped in 2024.
Yesterday evening it took 2 hours to code review the PRs, pull them, build them, deploy them in my HA, test them and then write reviews. Something is not panning out here

Edit: and again, no offence, I see you are trying, and I dont say there is any fault on you. I just dont understand what goes wrong, and can hopefully help. I review PRs and code for a living, and am writing code for many years no as well!

@ildar170975
Copy link
Copy Markdown
Collaborator Author

ildar170975 commented Apr 11, 2026 •
edited
Loading

Sorry, do you have particular comments for PRs? I mean comments to a code. Please do not consider me as rude, but comments for particular pieces of code and testing is what we need.

@Liquidmasl
Copy link
Copy Markdown

Liquidmasl commented Apr 12, 2026 •
edited
Loading

Sorry, do you have particular comments for PRs? I mean comments to a code. Please do not consider me as rude, but comments for particular pieces of code and testing is what we need.

Both mentioned PRs got reviews from me. and others as well.
I also suggested closing this PR. because this PR does not need testing or reviewed code, since, as I said, it is a duplicate of other PRs which solve the issue in a better way. No need to waste more time here then.

I believe thats not what the repo needs though, at least not only. something else is not working out, as I mentioned. I am not sure you understand my confusion and questions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ildar170975 @akloeckner @Nickduino @Liquidmasl