Skip to content

Conversation

michelinewu
Copy link
Contributor

No description provided.

Copy link

bundlemon bot commented Nov 17, 2023

BundleMon

Files updated (1)
Status Path Size Limits
renderer.(hash).js
6.63MB (+30.74KB +0.45%) -
Unchanged files (3)
Status Path Size Limits
vendors~renderer.(hash).js
4.65MB -
updater.js
115.28KB -
guest-api.js
40.19KB -

Total files change +30.74KB +0.26%

Final result: ✅

View report in BundleMon website ➡️


Current branch size history | Target branch size history

Copy link
Contributor

@blackxored blackxored left a comment

Choose a reason for hiding this comment

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

Most of the changes requested are cosmetic. The missing translations are probably the most important. The RxJS behavior following after, but it seems to be working correctly, so definitely up to you whether to refactor would provide value.

<>REC</>
)}
</span>
<span>{recordingStopping ? <i className="fa fa-spinner fa-pulse" /> : <>REC</>}</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: Should REC be translated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is how it was originally but you bring up a good point. The limitation is that there is only room for three characters on the button. Let's bring it up to the team?

Copy link
Contributor

Choose a reason for hiding this comment

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

yea the character limit is the issue here, as well as i'm not certain other languages have the same shorthand that we do for this term. i think looking into an icon replacement could have value here, as there is a generally accepted icon for recording


const { Option } = Select;

const recordingQualities = [
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe these should be translated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

// Refactor when migrate to new API, sleep to allow state to update
await new Promise(resolve => setTimeout(resolve, 300));
this.toggleRecording();
signalChanged.unsubscribe();
Copy link
Contributor

Choose a reason for hiding this comment

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

If I remember correctly RxJS didn't play nicely when unsubscribing to a subject inside a subscribe function. I believe this could be achieved with take(1) or takeWhile(condition) and then subscribe which would automatically unsubscribe for us.
Bear in mind is been a while so API might have changed or even this assumption.

this.signalInfoChanged.pipe(
  take(1)
 ).subscribe(...)

I believe the filter (if takeWhile isn't needed after all, it might) and the delay could also be achieved with operators, but that's not a real concern, just the subscribe/unsubscribe behavior is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just for tracking, discussed

// Refactor when move streaming to new API
if (this.state.verticalStreamingStatus !== EStreamingState.Offline) {
this.SET_VERTICAL_STREAMING_STATUS(EStreamingState.Offline);
}
signalChanged.unsubscribe();
Copy link
Contributor

Choose a reason for hiding this comment

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

RxJS unsubscribe comment earlier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed

}
}, 2000),
);
recordingStopped.unsubscribe();
Copy link
Contributor

Choose a reason for hiding this comment

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

RxJS unsubscribe comment earlier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed


:global(.ant-form-item-label) {
display: none;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

not worth requiring a change here but just a lil useful thing-- we have a boolean property nowrap on all of our react inputs that gets rid of the label portion

}

return true;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

just to follow the logic chain here, users can't stream and record to vertical simultaneously?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants