New Card: Shopping List by iantrich · Pull Request #1970 · home-assistant/frontend

iantrich · 2018-11-03T04:09:02Z

Following features:

Add item
Edit item
Complete item
Clear completed items

Following features: - Add item - Edit item - Complete item - Clear items

zsarnett · 2018-11-03T05:22:42Z

Not sure I like Clear completed I think Clear Checked would be a better phrasing. If its a shopping list or just a list and not actions to do then completed doesn't make sense.

I also think a notification-clear-all icon at the top for this action would be better personally.

src/panels/lovelace/cards/hui-shopping-list-card.ts

zsarnett · 2018-11-03T05:30:19Z

src/panels/lovelace/cards/hui-shopping-list-card.ts

+class HuiShoppingListCard extends hassLocalizeLitMixin(LitElement)
+  implements LovelaceCard {
+  public hass?: HomeAssistant;
+  protected _config?: Config;


Should be private,

iantrich · 2018-11-04T00:21:48Z

Not sure I like Clear completed I think Clear Checked would be a better phrasing. If its a shopping list or just a list and not actions to do then completed doesn't make sense.

I also think a notification-clear-all icon at the top for this action would be better personally.

@zsarnett

Added an icon to top right to replace complete button at bottom
Updated tooltip text to 'Clear checked items'
Aligned checkboxes with title
'Clear checked' icon on top right
- Placement feels off. Currently lined up with bottom of title text. There is also the issue of how this should look if there is no title defined. Currently with how I'm placing it, it would be off the card, but even compoensated for, it could collide with a row unless I add more padding to the top when there is no title. Or should I just require a title for the list?

zsarnett · 2018-11-04T04:11:02Z

What about something like this

Adding the second headar (smaller) with the icon next to that

balloob · 2018-11-04T13:04:02Z

src/data/shopping-list.ts

+    name,
+  });
+
+export const clearItems = (hass: HomeAssistant): Promise<void> =>


clearItems -> clearCompletedItems

balloob · 2018-11-04T13:06:13Z

src/panels/lovelace/cards/hui-shopping-list-card.ts

+  public connectedCallback(): void {
+    super.connectedCallback();
+
+    if (this.hass) {


So since you have this check, it is important that in case we get connected to the DOM and then get hass, we recover from that?

balloob · 2018-11-04T13:07:15Z

src/panels/lovelace/cards/hui-shopping-list-card.ts

+    super.connectedCallback();
+
+    if (this.hass) {
+      this.hass.connection


Make this this._unsubEvents = this.hass.connection.subscribeEvents(…) and then in disconnected do this:

if (this._unsubEvents) { this._unsubEvents.then(unsub => unsub()); }

That way, if the shopping card disconnects before the subscription is in place, we still unsubscribe.

balloob · 2018-11-04T13:07:34Z

src/panels/lovelace/cards/hui-shopping-list-card.ts

+      <ha-card .header="${this._config.title}">
+        <ha-icon
+          id="clear"
+          icon="hass:notification-clear-all"


balloob · 2018-11-04T13:08:50Z

src/panels/lovelace/cards/hui-shopping-list-card.ts

+      ${repeat(
+        this._items!,
+        (item) =>
+          !item.complete


Just filter before passing it to the render function: this._items.filter(item => !item.complete)

Also add an identification function to repeat as 2nd arg: item => item.id

balloob · 2018-11-04T13:12:03Z

src/panels/lovelace/cards/hui-shopping-list-card.ts

+      ${repeat(
+        this._items!,
+        (item) =>
+          item.complete


use filter and add identify func.

balloob · 2018-11-04T13:13:45Z

src/panels/lovelace/cards/hui-shopping-list-card.ts

+  }
+
+  private _addItem(ev): void {
+    if (this.shadowRoot!.querySelector("#addBox")) {


Don't ever write IFs that span whole or most of a function. Convert those to a guard clause:

const addBox = this.shadowRoot!.querySelector("#addBox"); if (!addBox) { return; } …

balloob · 2018-11-04T13:14:41Z

src/translations/en.json

      "script": {
        "execute": "Execute"
      },
+      "shopping-list": {


We should have a Lovelace section in the translation files.

Addressed review comments and scaled this back to just get a simple shopping list card out there and we can discuss/debate how best to add the additional pieces with smaller PRs

zsarnett · 2018-11-05T02:43:28Z

I don't think we should limit this card to shopping List. I think List Card may be a better name for all around use

iantrich · 2018-11-05T02:44:58Z

I don't think we should limit this card to shopping List. I think List Card may be a better name for all around use

The component is 'Shopping List'. And I already made a list-card :)
https://github.com/custom-cards/list-card

zsarnett · 2018-11-05T02:46:04Z

The component is 'Shopping List'. And I already made a list-card :)
https://github.com/custom-cards/list-card

Ahh Gotcha. Fair enough.

iantrich · 2018-11-05T02:46:54Z

The component is 'Shopping List'. And I already made a list-card :)
https://github.com/custom-cards/list-card

Ahh Gotcha. Fair enough.

In the future I plan to update the shopping list component to support multiple lists and would probably propose a renaming, but for MVP, I think this fits

balloob · 2018-11-06T09:25:09Z

src/panels/lovelace/cards/hui-shopping-list-card.ts

+    this._hass = hass;
+
+    if (!this._unsubEvents) {
+      this.connectedCallback();


This is dangerous, this calls all the super connected callbacks too. Extract the subbing of events into a method and call that here.

balloob · 2018-11-06T09:28:59Z

I've removed the calling of connectedCallback inside the hass setter as it had two unintended consequences:

we ended up calling connectedCallback of all super classes
we ended up subscribing before connected, and then when we got connected, we would subscribe a second time.

If I missed something or broke something, we can fix it in another PR. Yay for dev branch.

New Card: Shopping List

ddf923f

Following features: - Add item - Edit item - Complete item - Clear items

homeassistant added the cla-signed label Nov 3, 2018

ghost assigned iantrich Nov 3, 2018

ghost added the in progress label Nov 3, 2018

iantrich mentioned this pull request Nov 3, 2018

Shopping List Card documentation home-assistant/home-assistant.io#7346

Merged

2 tasks

Address Travis complaint

90bfbd2

zsarnett reviewed Nov 3, 2018

View reviewed changes

src/panels/lovelace/cards/hui-shopping-list-card.ts Outdated Show resolved Hide resolved

zsarnett reviewed Nov 3, 2018

View reviewed changes

src/panels/lovelace/cards/hui-shopping-list-card.ts Show resolved Hide resolved

zsarnett reviewed Nov 3, 2018

View reviewed changes

Addressed review comments

69f46b7

iantrich added 2 commits November 3, 2018 19:22

Update translation variable name

6ee8cf4

Line up input row text

f12c7be

balloob reviewed Nov 4, 2018

View reviewed changes

Taking MVP to heart

eea0a65

Addressed review comments and scaled this back to just get a simple shopping list card out there and we can discuss/debate how best to add the additional pieces with smaller PRs

balloob reviewed Nov 6, 2018

View reviewed changes

Remove calling connected in set hass

7989f5e

ghost assigned balloob Nov 6, 2018

balloob approved these changes Nov 6, 2018

View reviewed changes

balloob merged commit 6432207 into home-assistant:dev Nov 6, 2018

ghost removed the in progress label Nov 6, 2018

iantrich deleted the shopping branch November 16, 2018 19:17

balloob mentioned this pull request Nov 21, 2018

Merge 20181121.0 to master #2077

Merged

github-actions bot locked and limited conversation to collaborators Jul 7, 2022

Conversation

iantrich commented Nov 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zsarnett commented Nov 3, 2018

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iantrich commented Nov 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zsarnett commented Nov 4, 2018

Uh oh!

balloob Nov 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zsarnett commented Nov 5, 2018

Uh oh!

iantrich commented Nov 5, 2018

Uh oh!

zsarnett commented Nov 5, 2018

Uh oh!

iantrich commented Nov 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

balloob commented Nov 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

iantrich commented Nov 3, 2018 •

edited

Loading

iantrich commented Nov 4, 2018 •

edited

Loading

balloob Nov 4, 2018 •

edited

Loading