Add display type to notifications to allow snackbar to be shown. #1493

Merged

dan1d merged 1 commit from issue-1488-snackbars-notifications into master

2018-05-19 15:49:56 +02:00

dan1d commented

2018-05-18 19:15:39 +02:00

(Migrated from github.com)

Closes issue #1488, I've searched on the entire codebase to check if there is any place where doNotify was being called without displayType property(thanks @tzarebczan, your comment helped a lot! ).
@seanyesmunt I wonder if we should add a default displayType(ex: ["snackbars"]) in case someone forgets to add it while calling the function doNotify, anyway currently I've not added any default and just fixed the issue reported on #1488

This issue on #1488 :

Another user had reported something similar on the show page - 
he was unable to click the delete button (could click, but not action), until a restart.

Was also related to the notifications being queued but not dispatched because the displayType was not set, I could reproduce and tested the fix.

Closes issue #1488, I've searched on the entire codebase to check if there is any place where `doNotify` was being called without `displayType` property(thanks @tzarebczan, your comment helped a lot! ). @seanyesmunt I wonder if we should add a default `displayType`(ex: `["snackbars"]`) in case someone forgets to add it while calling the function `doNotify`, anyway currently I've not added any default and just fixed the issue reported on #1488 This issue on #1488 : ``` Another user had reported something similar on the show page - he was unable to click the delete button (could click, but not action), until a restart. ``` Was also related to the notifications being queued but not dispatched because the `displayType` was not set, I could reproduce and tested the fix.

neb-b (Migrated from github.com) reviewed 2018-05-18 19:15:39 +02:00

tzarebczan commented

2018-05-18 20:12:06 +02:00

(Migrated from github.com)

Thanks again for the quick response and resolution @dan1d ! I'll let @seanyesmunt comment on the defaulting scenario.

neb-b commented

2018-05-18 20:12:48 +02:00

(Migrated from github.com)

Thanks Dan. I think a default could work, but since everything is finally moved over, we should be able to check if new doNotify contain it in future PRs. But it's still something to think about!

kauffj commented

2018-05-18 20:48:31 +02:00

(Migrated from github.com)

Rather than a default would it make more sense to just error out or except if the expected/required value is not provided?

👍 1

neb-b commented

2018-05-18 20:49:59 +02:00

(Migrated from github.com)

That's pobably a better solution

On Fri, May 18, 2018, 2:48 PM Jeremy Kauffman notifications@github.com
wrote:

Rather than a default would it make more sense to just error out or except
if the expected/required value is not provided?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/lbryio/lbry-app/pull/1493#issuecomment-390298790, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AQGcjjMVuo5oLd27arWEoqLBxoEEYtiwks5tzxeFgaJpZM4UFE1b
.

That's pobably a better solution On Fri, May 18, 2018, 2:48 PM Jeremy Kauffman <notifications@github.com> wrote: > Rather than a default would it make more sense to just error out or except > if the expected/required value is not provided? > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <https://github.com/lbryio/lbry-app/pull/1493#issuecomment-390298790>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/AQGcjjMVuo5oLd27arWEoqLBxoEEYtiwks5tzxeFgaJpZM4UFE1b> > . >