Skip to content

Add Grid card#7476

Merged
bramkragten merged 12 commits intodevfrom
grid-card
Oct 29, 2020
Merged

Add Grid card#7476
bramkragten merged 12 commits intodevfrom
grid-card

Conversation

@balloob
Copy link
Member

@balloob balloob commented Oct 25, 2020

Breaking change

Proposed change

Add a new grid card. Config is like vertical/horizontal stack with an added optional columns count (default to 3) and a square option (default to true).

If square option is given, it will make sure all cells are square. This will make sure that 2 grid cards in different columns have the same height/width.

type: grid
columns: 3
square: false
cards:
  - 

2 cards with square: true (default):

image

2 cards with square: false

image

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Comment on lines +49 to +52
var(--grid-card-column-count, ${DEFAULT_COLUMNS}),
minmax(0, 1fr)
);
grid-gap: var(--grid-card-gap, 8px);
Copy link
Member Author

Choose a reason for hiding this comment

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

--grid-card-column-count is defined if columns is added to config, otherwise removed and it will take whatever is used.

--grid-card-gap cannot be defined from the config, relies on users setting their own CSS vars.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just hard code the number in there. The JS in the Static CSS I though was no good?

Copy link
Contributor

Choose a reason for hiding this comment

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

also, then the variable can be overwritten in a theme which is not something we want right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why wouldn't we want a theme to change it? It's up to the user to decide to do so.

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't want a theme to change it if the card can. Doesn't seem like the correct way having two ways that can change it

Copy link
Contributor

Choose a reason for hiding this comment

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

The gap I understand. I think that the grid column config should only be apart of the card config

#root {
display: grid;
grid-template-columns: repeat(
var(--grid-card-column-count, ${DEFAULT_COLUMNS}),
Copy link
Contributor

Choose a reason for hiding this comment

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

Variable css in the static styles? I thought this was frowned upon?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not variable, it's a constant defined at the top. Inside static styles it's fine, as it is only fetched and processed once. You should not have dynamic CSS inside a render block.

Copy link
Contributor

@zsarnett zsarnett left a comment

Choose a reason for hiding this comment

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

Is this a card that should be added to the Card Picker? If so, we need to add it to src\panels\lovelace\editor\lovelace-cards.ts

@balloob
Copy link
Member Author

balloob commented Oct 27, 2020

I think that with the grid card we can phase out the horizontal and vertical stack cards.


return (
maxCardSize *
(this._cards.length / (this._config.columns || DEFAULT_COLUMNS))
Copy link
Member

Choose a reason for hiding this comment

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

Should we limit the columns to the number of cards when there are less cards than columns?

Suggested change
(this._cards.length / (this._config.columns || DEFAULT_COLUMNS))
(this._cards.length / (this._config.columns || (Math.min(DEFAULT_COLUMNS, this._cards.length)))

Copy link
Member

Choose a reason for hiding this comment

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

^ thats not going it work reading the code below but still... 😛

Copy link
Member Author

Choose a reason for hiding this comment

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

So if we want that, we can't expose the CSS var. I'm fine either way. What do you prefer?

Copy link
Member

Choose a reason for hiding this comment

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

I think it is nice for a user to not have to worry about the columns setting, and if you add 2 cards you don't get a blank space. But also to make it work like our current horizontal stack.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that it depends on if you want this. For example, if I have 2 grid cards next to one another, I want all cards to be the same size!

image

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh I guess we already don't have that now
image

Copy link
Member Author

Choose a reason for hiding this comment

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

Back to the drawing board. I think that we should make sure that the height is included (but removable maybe?) to have multiple grid cards look the same.

Copy link
Member Author

Choose a reason for hiding this comment

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

Better:
image

@balloob
Copy link
Member Author

balloob commented Oct 28, 2020

Thought about it more and decided to not reduce default column count to the number of cards if that is smaller, because it doens't work well:

image

Just setting it to default columns if none is set, gives a better result:

image

@bramkragten bramkragten merged commit c3718ff into dev Oct 29, 2020
@bramkragten bramkragten deleted the grid-card branch October 29, 2020 09:31
@frenck
Copy link
Member

frenck commented Oct 29, 2020

Docs missing?

@balloob
Copy link
Member Author

balloob commented Oct 29, 2020

@iantrich
Copy link
Member

I think that with the grid card we can phase out the horizontal and vertical stack cards.

That would currently be a breaking change which we shouldn't do as Lovelace is now considered 1.0.

Even if you intend to take defined horizontal/vertical cards and render them with grid, you are missing title in the current form of grid.

@balloob
Copy link
Member Author

balloob commented Oct 30, 2020

Sorry, my bad. With phasing out I didn't mean remove but don't promote in the card picker.

I actually have to also retract that statement, I think that vertical stack still has a use case in case you want to make sure 2 cards always get grouped together.

@bramkragten bramkragten mentioned this pull request Nov 11, 2020
This was referenced Nov 11, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Jul 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants