Skip to content

Conversation

@herra
Copy link

@herra herra commented May 25, 2020

  • UI issues

@groupsky groupsky marked this pull request as draft May 27, 2020 10:29
@herra herra mentioned this pull request May 28, 2020
Draft
34 tasks
@herra herra force-pushed the feature/add-toast-to-notify-user branch from c2a163b to ef18c2b Compare May 28, 2020 11:00
@herra herra changed the title Add toast component to show information to user [WIP] Add toast component to show information to user May 28, 2020
@herra herra marked this pull request as ready for review May 28, 2020 14:49
@herra herra requested a review from groupsky May 28, 2020 14:50
Copy link

@groupsky groupsky left a comment

Choose a reason for hiding this comment

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

It's an improvement over the existing alert.

@@ -1,4 +1,5 @@
<div class="container">
<toast-component></toast-component>

Choose a reason for hiding this comment

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

If moved to the bottom would implicitly get higher z-index

Comment on lines 50 to 51
this.toast.classList.add('hide')
this.toast.classList.remove('show')

Choose a reason for hiding this comment

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

Duplicated in 3 places - better to extract as a method

Comment on lines 37 to 39
const toastTitle = this.shadowRoot.querySelector('#title')
const toastMessage = this.shadowRoot.querySelector('#message')
const toastTimestamp = this.shadowRoot.querySelector('#time')

Choose a reason for hiding this comment

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

could be memoized

Co-authored-by: Geno Roupsky <geno@roupsky.name>
@lgtm-com
Copy link

lgtm-com bot commented May 29, 2020

This pull request introduces 1 alert when merging 4048dc1 into 8012a85 - view on LGTM.com

new alerts:

  • 1 for Syntax error

@herra herra requested a review from groupsky May 29, 2020 07:11
@groupsky groupsky marked this pull request as draft June 29, 2020 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants