Skip to content

Add onClick listener to dismiss toast notification.#7268

Merged
bramkragten merged 1 commit intohome-assistant:devfrom
gilsonmandalogo:dismissToastOnClick
Oct 21, 2020
Merged

Add onClick listener to dismiss toast notification.#7268
bramkragten merged 1 commit intohome-assistant:devfrom
gilsonmandalogo:dismissToastOnClick

Conversation

@gilsonmandalogo
Copy link
Copy Markdown
Contributor

Proposed change

Dismiss toast notification on click to prevent blocking user accessing some menu buttons (like profile button).

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

No configuration needed.

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:

@zsarnett
Copy link
Copy Markdown
Contributor

zsarnett commented Oct 8, 2020

We should add a Dismiss button to all of them. Adding this will add two different ways of dismissing some of our toasts

@gilsonmandalogo
Copy link
Copy Markdown
Contributor Author

@zsarnett Check if now makes sense, please.

Comment on lines +72 to +75
<mwc-button
.label=${this.hass.localize("ui.notification_toast.dismiss")}
@click=${this.dismiss}
></mwc-button>
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.

The dismissal action should be passed as an action from the caller. We should not just add a button on every toast.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok, if I understand you, I added a default action to only dismiss toast, and now we will always have only one button. Take a look, please.

Comment on lines +54 to +56
this._action = {
text: this.hass.localize("ui.notification_toast.dismiss"),
};
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.

This part should move to the caller of the toast, we should be able to decide per toast if it is dismissable. Like the connection lost toast should not be dismissable

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should make it so that the dismiss is optional but on by default no?

I think a toast should usually be dismissable and should only not be dismissable in fewer cases.

Adding this as the default I think is good.

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 most toasts, it is not needed as they will be hidden automatically.

Also material guidelines:

image

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

They will be hidden after X time. But you have to wait form that. Also, just a caution guideline 😉

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe the onClick was the best idea instead of a button action on each. In the Google Photos App. If I delete an image. I can click the toast to dismiss it. as well as there is an action to undo the action taken.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@zsarnett @bramkragten So, should I revert to toast onClick or add a dismiss button on some toasts?

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.

Add a dismiss button for some toasts, like the HA is starting one. We should not allow dismissing them all.

Copy link
Copy Markdown
Contributor Author

@gilsonmandalogo gilsonmandalogo Oct 12, 2020

Choose a reason for hiding this comment

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

@bramkragten Ok, is there any more toasts besides that one to add a dismiss button?

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.

Not sure, you can search the code base for showToast

@bramkragten bramkragten merged commit 4a5935e into home-assistant:dev Oct 21, 2020
@bramkragten bramkragten mentioned this pull request Oct 21, 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.

4 participants