Skip to content
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

Add delete button to MQTT devices' config page #5117

Merged
merged 6 commits into from Mar 20, 2020

Conversation

emontnemery
Copy link
Collaborator

Proposed change

Add delete button to MQTT devices' config page
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

Additional information

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:

@@ -1480,7 +1480,9 @@
"area": "Area",
"integration": "Integration",
"battery": "Battery"
}
},
"delete": "DELETE",
Copy link
Member

Choose a reason for hiding this comment

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

Keeps this lowercase, the styling will make it uppercase

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.
I also changed some other button labels to lowercase.

@@ -81,6 +84,18 @@ export class HaDeviceCard extends LitElement {
</div>
`
: ""}
${this.domains.includes("mqtt")
Copy link
Member

Choose a reason for hiding this comment

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

Let's not add this in ha-device-card but instead, make a separate card specific for mqtt options.


static get styles(): CSSResult {
return css`
ha-card {
Copy link
Member

Choose a reason for hiding this comment

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

There is no ha-card in this element

padding-bottom: 10px;
min-width: 0;
}
.device {
Copy link
Member

Choose a reason for hiding this comment

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

There is no element with the class device

Comment on lines 27 to 28
<div class="info">
<div class="buttons">
Copy link
Member

Choose a reason for hiding this comment

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

These classes are not defined, so probably the div is not needed.

Comment on lines 20 to 23
@property() public devices!: DeviceRegistryEntry[];
@property() public areas!: AreaRegistryEntry[];
@property() public narrow!: boolean;
@property() public domains!: string[];
Copy link
Member

Choose a reason for hiding this comment

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

Not used?

Comment on lines 168 to 169
.areas=${this.areas}
.devices=${this.devices}
Copy link
Member

Choose a reason for hiding this comment

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

Not used?

Comment on lines 28 to 31
protected firstUpdated(changedProps) {
super.firstUpdated(changedProps);
}

Copy link
Member

Choose a reason for hiding this comment

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

Not used, can be removed.

Suggested change
protected firstUpdated(changedProps) {
super.firstUpdated(changedProps);
}

Comment on lines 36 to 38
private _confirmDeleteEntry(): void {
showConfirmationDialog(this, {
text: this.hass.localize("ui.panel.config.devices.confirm_delete"),
confirm: () => this._deleteEntry(),
});
}
Copy link
Member

Choose a reason for hiding this comment

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

showConfirmationDialog returns a promise that resolves to true if user confirmed. So you can do something like

const confirmed = await showConfirmationDialog()

if (!confirmed) { return; }

await removeMQTTDeviceEntry(this.hass!, this.device.id);

static get styles(): CSSResult {
return css`
mwc-button.warning {
margin-right: auto;
Copy link
Member

Choose a reason for hiding this comment

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

That seems weird to add here. Why is that ?

Also the coloring is already available in haStyle or something, I think ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I stole it from here:

mwc-button.warning {
margin-right: auto;
--mdc-theme-primary: var(--google-red-500);
}

Do you mean to use this instead:

--error-color: #db4437;

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

The margin-right: auto is to pull the secondary button to the left in a dialog normally

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! Fixed now.

@bramkragten bramkragten merged commit c54f2b6 into home-assistant:dev Mar 20, 2020
@lock lock bot locked and limited conversation to collaborators Mar 21, 2020
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.

None yet

4 participants