Skip to content

Add card functionality#2160

Merged
balloob merged 8 commits intodevfrom
add-card
Dec 3, 2018
Merged

Add card functionality#2160
balloob merged 8 commits intodevfrom
add-card

Conversation

@bramkragten
Copy link
Copy Markdown
Member

@bramkragten bramkragten commented Nov 30, 2018

Very much WIP, dirty and buggy.
addcard3
Closes: #1960

@ghost ghost assigned bramkragten Nov 30, 2018
@ghost ghost added the in progress label Nov 30, 2018
@bramkragten bramkragten changed the title MVP add card WIP: MVP add card Nov 30, 2018
@zsarnett
Copy link
Copy Markdown
Contributor

Can we get a Gif of the process when you get it to a good point.

@iantrich
Copy link
Copy Markdown
Member

iantrich commented Dec 1, 2018

fixes #1960

<h3>Pick the card you want to add:</h3>
<div class="cards-container">
${
cards.map((card) => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Idea: In the future we can add a global window object where custom cards can register so they can be shown in this list too.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah! I had that in mind!

private _configValue?: ConfigValue;
private _configState?: string;
private _loading?: boolean;
private _isToggleAvailable?: boolean;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we need to store this as an instance variable or can this be derived from other instance values?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should aim to store as little instance variables as possible, all values that can be derived from others, should not be stored. If used at multiple places consider adding a getter that computes it on the fly.

The main reason for this is single source of truth. What if isToggleAvailable goes out of sync with the values it was computed off, what happens then? By computing it on the fly we don't have this problem.

}

private async _loadConfigElement(conf: LovelaceCardConfig): Promise<void> {
private async _loadConfigElement(conf: LovelaceCardConfig): Promise<boolean> {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For a future PR, we should look if we can split out the editor piece from the editor dialog/data handling piece. A card editor can take a CardConfig and PreferredUI (YAML/UI). All the async loading of elements and which editor to show can just be in that element. The dialog element can still show the toggle YAML button to control the editor, or that can be moved into the editor element too (under a … menu)

@bramkragten bramkragten changed the title WIP: MVP add card Add card functionality Dec 3, 2018
@balloob
Copy link
Copy Markdown
Member

balloob commented Dec 3, 2018

🎉 🎉 🎉 🎉 🎉

@balloob balloob merged commit f461ad6 into dev Dec 3, 2018
@delete-merged-branch delete-merged-branch bot deleted the add-card branch December 3, 2018 13:11
@ghost ghost removed the in progress label Dec 3, 2018
@ghost
Copy link
Copy Markdown

ghost commented Dec 3, 2018

Please also add strings to translation to we can start working on translating.

@balloob balloob mentioned this pull request Dec 5, 2018
@ghost
Copy link
Copy Markdown

ghost commented Dec 7, 2018

Please add to option add card also option to set ID. Now ID will be generate automatical.

@bramkragten
Copy link
Copy Markdown
Member Author

This is not a place for feature requests or support. You can create an issue for that. You can change the ID in the YAML editor for cards.

@ghost
Copy link
Copy Markdown

ghost commented Dec 7, 2018

Yes but when we add editor UI better will be edit in UI.

@github-actions github-actions bot locked and limited conversation to collaborators Jul 7, 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.

Allow adding a new LL card

5 participants