Random fixes #2729

					
				@ -88,1 +108,4 @@

				    });

				  }

				  const hidePlayer = !isPlaying || !uri || (!inline && (!floatingPlayerEnabled || !isStreamable));

neb-b (Migrated from github.com) commented

2019-08-14 20:12:29 +02:00

@tzarebczan brought up that we might not want to do play time (or keep play time separate) for non-streamable content.

kauffj (Migrated from github.com) reviewed 2019-08-15 00:31:23 +02:00

src/ui/analytics.js

					
				@ -99,2 +99,4 @@

				  rewardEligibleEvent: () => {

				    sendGaEvent('Engagement', 'Reward-Eligible');

				  },

				};

kauffj (Migrated from github.com) commented

2019-08-15 00:26:25 +02:00

Making a sendReactGAEvent or equivalent function would eliminate these repeated if checks and could also help ensure the checks are not accidentally left out in the future.

Making a `sendReactGAEvent` or equivalent function would eliminate these repeated if checks and could also help ensure the checks are not accidentally left out in the future.

src/ui/component/app/view.jsx

kauffj (Migrated from github.com) commented

2019-08-15 00:26:55 +02:00

=== undefined? or just !previousUserId ?

`=== undefined`? or just `!previousUserId` ?

kauffj (Migrated from github.com) commented

2019-08-15 00:27:14 +02:00

same question for below undefined checks

same question for below `undefined` checks

src/ui/component/fileViewer/view.jsx

					
				@ -88,0 +103,4 @@

				    const newX = x + ui.deltaX;

				    const newY = y + ui.deltaY;

				    setPosition({

				      x: newX,

kauffj (Migrated from github.com) commented

2019-08-15 00:28:26 +02:00

😂

:joy:

src/ui/component/fileViewer/view.jsx

					
				@ -88,1 +108,4 @@

				    });

				  }

				  const hidePlayer = !isPlaying || !uri || (!inline && (!floatingPlayerEnabled || !isStreamable));

kauffj (Migrated from github.com) commented

2019-08-15 00:28:05 +02:00

I'm not particularly concerned about this, I think it can be cleaned up in internal analytics.

src/ui/modal/modalAutoGenerateThumbnail/index.js

					
				@ -7,3 +8,3 @@

				  closeModal: () => dispatch(doHideModal()),

				  upload: buffer => dispatch(doUploadThumbnail(null, buffer)),

				  upload: buffer => dispatch(doUploadThumbnail(null, buffer, null, fs)),

				  showToast: options => dispatch(doToast(options)),

kauffj (Migrated from github.com) commented

2019-08-15 00:29:01 +02:00

should these 3rd and 4th params be re-ordered? (meh)

src/ui/modal/modalConfirmThumbnailUpload/index.js

					
				@ -7,3 +8,3 @@

				  closeModal: () => dispatch(doHideModal()),

				  upload: path => dispatch(doUploadThumbnail(path)),

				  upload: path => dispatch(doUploadThumbnail(path, null, null, fs)),

				  updatePublishForm: value => dispatch(doUpdatePublishForm(value)),

kauffj (Migrated from github.com) commented

2019-08-15 00:29:31 +02:00

same question as above, if fs is regularly provided but other params are not, consider moving fs up in param order

when does JS get named parameters?!

same question as above, if `fs` is regularly provided but other params are not, consider moving fs up in param order when does JS get named parameters?!

src/ui/redux/actions/app.js

kauffj (Migrated from github.com) commented

2019-08-15 00:30:11 +02:00

could be:

if (!claimIsMine) { logView() }

could be: `if (!claimIsMine) { logView() }`

src/ui/scss/component/_button.scss

					
				@ -95,2 +95,4 @@

				}

				// Quick fix because this is a pain

				// There is something weird with wrapping buttons. Some places we want to wrap and others we want to ellips

kauffj (Migrated from github.com) commented

2019-08-15 00:30:46 +02:00

Good job noting why the rules are being broken so a future dev and/or future you can understand

src/ui/scss/init/_gui.scss

					
				@ -99,2 +99,4 @@

				}

				img,

				a {

kauffj (Migrated from github.com) commented

2019-08-15 00:31:16 +02:00

Confident in no consequences to this rule being so universal?

neb-b (Migrated from github.com) reviewed 2019-08-15 04:57:16 +02:00

src/ui/component/app/view.jsx

neb-b (Migrated from github.com) commented

2019-08-15 04:57:16 +02:00

I guess this one specifically could be just be !previousUserId but the other ones need to check that they weren't undefined so the event isn't fired from the initial user fetch

I guess this one specifically could be just be `!previousUserId` but the other ones need to check that they weren't `undefined` so the event isn't fired from the initial user fetch

neb-b (Migrated from github.com) reviewed 2019-08-15 04:57:51 +02:00

src/ui/modal/modalAutoGenerateThumbnail/index.js

					
				@ -7,3 +8,3 @@

				  closeModal: () => dispatch(doHideModal()),

				  upload: buffer => dispatch(doUploadThumbnail(null, buffer)),

				  upload: buffer => dispatch(doUploadThumbnail(null, buffer, null, fs)),

				  showToast: options => dispatch(doToast(options)),

neb-b (Migrated from github.com) commented

2019-08-15 04:57:51 +02:00

The third param is the android equivalent of fs

The third param is the android equivalent of `fs`

neb-b (Migrated from github.com) reviewed 2019-08-15 04:59:16 +02:00

src/ui/scss/init/_gui.scss

					
				@ -99,2 +99,4 @@

				}

				img,

				a {

neb-b (Migrated from github.com) commented

2019-08-15 04:59:16 +02:00

97% confidence

neb-b (Migrated from github.com) reviewed 2019-08-15 05:11:32 +02:00

src/ui/analytics.js

					
				@ -99,2 +99,4 @@

				  rewardEligibleEvent: () => {

				    sendGaEvent('Engagement', 'Reward-Eligible');

				  },

				};

neb-b (Migrated from github.com) commented

2019-08-15 05:11:31 +02:00

Yeah I've been meaning to do this, just did and it's way nicer now.

Sign in to join this conversation.

No reviewers

No labels

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-desktop#2729