feat: sections support #1139
feat: sections support #1139
Conversation
|
Please merge this into the base branch, guys, we need it :-) |
|
Honestly I made a quick look at this PR. IMHO we can give this PR a go & test with participation of users who is using sections. |
There was a problem hiding this comment.
As the discussion came here before I could look at the UI PR again, I used some travel time to review. I must admit, I don't use sections, so this is from a pure code review perspective now.
Forgive me if I didn't get some parts of the code completely. All suggestions are open to discussion.
src/main.js
Outdated
| @@ -127,6 +135,68 @@ class MiniGraphCard extends LitElement { | |||
| } | |||
| } | |||
|
|
|||
| checkSections() { | |||
| let root = document.querySelector('home-assistant'); | |||
There was a problem hiding this comment.
Is this really the best way to check for the view layout? The main frontend suggests, there might be this.layout?
Also, maybe this.options.layout_options will only defined in sections view?
There was a problem hiding this comment.
this.layout does not seem to be defined when the graphs are created in getConfig. I could try to modify graphs height in render as it seems that this.layout is defined there.
As for the this.options.layout_options there could be a situation where user copies card to a different dashboard that is not necessarily a sections but maybe I am overthinking.
src/main.js
Outdated
| } | ||
|
|
||
| getLayoutSize(layout) { | ||
| if (!this.sections) { |
There was a problem hiding this comment.
Reduce?
return this.sections && layout.grid_rows === 2 ? 'small' : '';And if that is enough, maybe remove the function and do the logic in the render directly... (Because it seems used only once.)
src/style.js
Outdated
| @@ -49,6 +53,9 @@ const style = css` | |||
| order: -1; | |||
| padding: 0 16px 8px 16px; | |||
| } | |||
| ha-card[fill].sections .graph__legend { | |||
There was a problem hiding this comment.
Why do the margins and paddings need tweaking? If it's kind of a refactor but yields the same layout, let's consider to change this also in non-section view to keep consistency.
| @@ -238,6 +254,10 @@ const style = css` | |||
| margin-top: auto; | |||
| width: 100%; | |||
| } | |||
| .sections .graph { | |||
There was a problem hiding this comment.
Here, too: if this is useful also in non-section views, let's consider to just add it to the regular styles.
There was a problem hiding this comment.
I think this is only useful for section view. It makes graph position itself so that it's not clipping out of bounds.
| return false; | ||
| } | ||
|
|
||
| getCurrentLayout() { |
There was a problem hiding this comment.
I wonder: isn't this taken care of by home assistant? If we report a minimum row count via getLayoutOptions, shouldn't this be reflected in this.config.layout_options?
See also here, maybe: https://github.com/home-assistant/frontend/blob/67217b9dd0f5ad43aafb3bba2fe2821407b25243/src/panels/lovelace/editor/card-editor/hui-card-layout-editor.ts#L68
There was a problem hiding this comment.
No, it only clamps the values for setting the card size and for the visual editor, values set by user in this.config.layout_options stays untouched even when they're lower than minimum set in getLayoutOptions.
Card is considered small when its row size is 3 or less Simpler sections checking Simpler card height style
|
Hi Guys! Thanks for your work, just wanted to let you guys know that as a sections user, i tried out this PR and it did fix my dashboards Before: After: Personally i did like the thinner lines, but thats just preference and i can just add line_width to my yaml ofcourse |
|
@jnnck Awesome. Could you show us how the labels look like on your humidity graph with the PR?
|
|
I haven't touched the labels, but I think it could be solved by changing This would introduce some overlaps, of course. Before: |
|
Hope this PR will only touch sections and will not affect other layouts. As for labels and similar: consider more playing with fonts than with placement. |
If you want to go that route, I think you shouldn't leave such an important gap at the bottom and the state should remain on top of the labels |
Labels before: Labels after: |
This adds support for sections dashboard and addresses #1111.
Which means it fixes card size and also add card size restrictions based on the card configuration.
This "shouldn't" affect other types of dashboards.