New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New Card: Shopping List #1970

Merged
merged 7 commits into from Nov 6, 2018

Conversation

Projects
None yet
4 participants
@iantrich
Contributor

iantrich commented Nov 3, 2018

Following features:

  • Add item
  • Edit item
  • Complete item
  • Clear completed items

shopping-list

New Card: Shopping List
Following features:
- Add item
- Edit item
- Complete item
- Clear items

@wafflebot wafflebot bot added the in progress label Nov 3, 2018

@iantrich iantrich referenced this pull request Nov 3, 2018

Merged

Shopping List Card documentation #7346

0 of 2 tasks complete
@zsarnett

This comment has been minimized.

Contributor

zsarnett commented Nov 3, 2018

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.

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

This comment has been minimized.

@zsarnett

zsarnett Nov 3, 2018

Contributor

Should be private,

@iantrich

This comment has been minimized.

Contributor

iantrich commented Nov 4, 2018

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?

image

iantrich added some commits Nov 4, 2018

@zsarnett

This comment has been minimized.

Contributor

zsarnett commented Nov 4, 2018

What about something like this
example

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

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

This comment has been minimized.

@balloob

balloob Nov 4, 2018

Member

clearItems -> clearCompletedItems

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

This comment has been minimized.

@balloob

balloob Nov 4, 2018

Member

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?

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

This comment has been minimized.

@balloob

balloob Nov 4, 2018

Member

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.

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

This comment has been minimized.

@balloob

balloob Nov 4, 2018

Member

.icon ?

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

This comment has been minimized.

@balloob

balloob Nov 4, 2018

Member

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

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

This comment has been minimized.

@balloob

balloob Nov 4, 2018

Member

use filter and add identify func.

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

This comment has been minimized.

@balloob

balloob Nov 4, 2018

Member

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; }

…
@@ -394,6 +394,10 @@
"script": {
"execute": "Execute"
},
"shopping-list": {

This comment has been minimized.

@balloob

balloob Nov 4, 2018

Member

We should have a Lovelace section in the translation files.

Taking MVP to heart
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

This comment has been minimized.

Contributor

zsarnett commented Nov 5, 2018

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

This comment has been minimized.

Contributor

iantrich commented Nov 5, 2018

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

This comment has been minimized.

Contributor

zsarnett commented Nov 5, 2018

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

Ahh Gotcha. Fair enough.

@iantrich

This comment has been minimized.

Contributor

iantrich commented Nov 5, 2018

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

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

This comment has been minimized.

@balloob

balloob Nov 6, 2018

Member

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

@balloob

This comment has been minimized.

Member

balloob commented Nov 6, 2018

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.

@balloob

balloob approved these changes Nov 6, 2018

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

3 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
deploy/netlify Deploy preview ready!
Details

@wafflebot wafflebot bot removed the in progress label Nov 6, 2018

@iantrich iantrich deleted the iantrich:shopping branch Nov 16, 2018

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