LBRYCommunity/lbry-desktop

Fork 0

Move all modal types to use lbry-redux #1384

New issue

Closed

opened 2018-04-25 00:18:56 +02:00 by neb-b · 8 comments

neb-b commented

2018-04-25 00:18:56 +02:00

(Migrated from github.com)

The Issue

A few places are still using constants/modal_types for modal strings in the app. These should all be moved over to lbry-redux
ex: https://github.com/lbryio/lbry-app/blob/master/src/renderer/modal/modalRouter/view.jsx#L23

Expected Behaviour

All modal types should be added to the lbry-redux repo and pulled from there

Actual Behaviour

Some are using lbry-redux others are using the constants in the app

Suggested Solutions

Copy all the constants in src/rendered/constants/modal_types to here https://github.com/lbryio/lbry-redux/blob/master/src/constants/modal_types.js

Then change the import statements inside the app

## The Issue A few places are still using `constants/modal_types` for modal strings in the app. These should all be moved over to `lbry-redux` ex: https://github.com/lbryio/lbry-app/blob/master/src/renderer/modal/modalRouter/view.jsx#L23 ### Expected Behaviour All modal types should be added to the `lbry-redux` repo and pulled from there ### Actual Behaviour Some are using `lbry-redux` others are using the constants in the app ### Suggested Solutions Copy all the constants in `src/rendered/constants/modal_types` to here https://github.com/lbryio/lbry-redux/blob/master/src/constants/modal_types.js Then change the import statements inside the app

kauffj commented

2018-04-26 17:13:39 +02:00

(Migrated from github.com)

@seanyesmunt @akinwale should these become notification_types rather than modal_types given the refactoring of lbry-redux notification methodology?

@seanyesmunt @akinwale should these become `notification_types` rather than `modal_types` given the refactoring of lbry-redux notification methodology?

dan1d commented

2018-05-29 04:49:28 +02:00

(Migrated from github.com)

Sorry to bother, but this one shouldn't be closed when it was merged?

neb-b commented

2018-05-31 06:35:23 +02:00

(Migrated from github.com)

Guess I missed your message @kauffj

Probably. They are passed in as notificationId.

Guess I missed your message @kauffj Probably. They are passed in as `notificationId`.

neb-b commented

2018-05-31 06:35:52 +02:00

(Migrated from github.com)

And yes @dan1d. I just saw this was still open.

I'll leave it open for now since we are still discussing.

And yes @dan1d. I just saw this was still open. I'll leave it open for now since we are still discussing.

tzarebczan commented

2018-06-21 07:56:26 +02:00

(Migrated from github.com)

@seanyesmunt were all these moved?

kauffj commented

2018-06-25 22:33:36 +02:00

(Migrated from github.com)

It's conceivable, likely even, that applications will desire their own notifications independent from those used cross-platform. If that case doesn't exist yet, it's probably fine to wait until it does, but it's something to consider when designing the pattern used here.

neb-b commented

2018-06-25 22:42:02 +02:00

(Migrated from github.com)

Whoops missed this. Yeah the app is using lbry-redux for all notifications

On Mon, Jun 25, 2018, 4:33 PM Jeremy Kauffman notifications@github.com
wrote:

It's conceivable, likely even, that applications will desire their own
notifications independent from those used cross-platform. If that case
doesn't exist yet, it's probably fine to wait until it does, but it's
something to consider when designing the pattern used here.

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

Whoops missed this. Yeah the app is using lbry-redux for all notifications On Mon, Jun 25, 2018, 4:33 PM Jeremy Kauffman <notifications@github.com> wrote: > It's conceivable, likely even, that applications will desire their own > notifications independent from those used cross-platform. If that case > doesn't exist yet, it's probably fine to wait until it does, but it's > something to consider when designing the pattern used here. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <https://github.com/lbryio/lbry-app/issues/1384#issuecomment-400086082>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AQGcjt5B4C5jOjeGNOD0mUwAPwx8_cNiks5uAUkigaJpZM4TigcI> > . >

tzarebczan commented

2018-06-28 21:02:14 +02:00

(Migrated from github.com)

This is all in lbry-redux now