Notifications v2 (part 1) #93

2018-10-29T06:07:11+01:00

neb-b commented

2018-10-29 06:07:11 +01:00

(Migrated from github.com)

Notes

This is the first of several PR's and is just to get the existing lbry-redux code working with the new notifications state. Future PR's will be solely additions (less bug fixes with this first PR).
- Only added actions for Toasts (since that is all lbry-redux uses)
- Will add actions for Events/Errors when I implement that in the app
Adoption for lbry-android should be straight forward and should only require a few variable name changes, lbry-desktop will be more work since it is using modals.
Checkout types/notification for the complete notification reducer types (and let me know if I missed anything. This was based on previous conversations)
There are a lot of paren changes due to a recent prettier config setting change

Changes

Remove modals from lbry-redux
doNotify({ ... displayType: ["toast"] }) becomes doToast({ ... })
selectNotification() becomes selectToast()

## Notes - This is the first of several PR's and is just to get the existing lbry-redux code working with the new notifications state. Future PR's will be solely additions (less bug fixes with this first PR). - Only added actions for Toasts (since that is all lbry-redux uses) - Will add actions for Events/Errors when I implement that in the app - Adoption for `lbry-android` should be straight forward and should only require a few variable name changes, `lbry-desktop` will be more work since it is using modals. - Checkout `types/notification` for the complete notification reducer types (and let me know if I missed anything. This was based on previous conversations) - There are a lot of paren changes due to a recent prettier config setting change ### Changes - Remove modals from lbry-redux - `doNotify({ ... displayType: ["toast"] })` becomes `doToast({ ... })` - `selectNotification()` becomes `selectToast()`

skhameneh (Migrated from github.com) reviewed 2018-10-30 19:24:32 +01:00

skhameneh (Migrated from github.com) left a comment

Minor comments with the exception of the routine with the circular reference (if it weren't for my lack of understanding I'd sign off)

src/redux/actions/claims.js

skhameneh (Migrated from github.com) commented

2018-10-30 19:19:05 +01:00

parens:

(claim) => (claim.txid === txid && claim.nout === nout)

parens: ``` (claim) => (claim.txid === txid && claim.nout === nout) ```

src/redux/reducers/notifications.js

skhameneh (Migrated from github.com) commented

2018-10-30 19:21:52 +01:00

Why the circular reference?
I don't entirely understand this routine.

Why the circular reference? I don't entirely understand this routine.

src/redux/selectors/notifications.js

					
				@ -31,0 +18,4 @@

				  if (state.errors.length) {

				    const { error } = state.errors[0];

				    return {

				      error,

skhameneh (Migrated from github.com) commented

2018-10-30 19:23:08 +01:00

Could be state.toasts.length !== 0, I'm mostly indifferent - style vs speed

Could be `state.toasts.length !== 0`, I'm mostly indifferent - style vs speed

neb-b (Migrated from github.com) reviewed 2018-11-12 19:02:35 +01:00

src/redux/reducers/notifications.js

neb-b (Migrated from github.com) commented

2018-11-12 19:02:35 +01:00

That was just a typo. Fixed

akinwale (Migrated from github.com) reviewed 2018-11-12 23:14:13 +01:00

src/types/Notification.js

					
				@ -19,0 +23,4 @@

				};

				export type DoToast = {

				  type: ACTIONS.CREATE_TOAST,

akinwale (Migrated from github.com) commented

2018-10-31 13:38:36 +01:00

Any particular reason for renaming to toasts? All toasts are notifications, but not all notifications are toasts.

EDIT: Just saw that there are three types of notifications, toasts, events and errors. I don't feel too strongly about this, but I think we could use a better name instead of events.

Just to add to this, I'm wary of using Event because we already have events in Javascript, so it may lead to some confusion for new code contributors not familiar with the codebase.

Any particular reason for renaming to toasts? All toasts are notifications, but not all notifications are toasts. EDIT: Just saw that there are three types of notifications, toasts, events and errors. I don't feel too strongly about this, but I think we could use a better name instead of events. Just to add to this, I'm wary of using `Event` because we already have events in Javascript, so it may lead to some confusion for new code contributors not familiar with the codebase.

skhameneh (Migrated from github.com) approved these changes 2018-11-14 20:55:54 +01:00

Sign in to join this conversation.

No reviewers

skhameneh

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: LBRYCommunity/lbry-redux#93