Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(modal): Port ServerIdentity (closes #112) #250

Closed

Conversation

aisuleep
Copy link

Please make sure to check the following tasks before opening and submitting a PR

  • I understand and have followed the contribution guide
  • I have tested my changes locally and they are working as intended
  • These changes do not have any notable side effects on other Revolt projects

@github-actions
Copy link

github-actions bot commented Jul 30, 2023

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@aisuleep
Copy link
Author

Not sure if still needed, but the issue was still labeled as "help-wanted" so I spruced up and finished the draft PR from before. 👍

@aisuleep
Copy link
Author

aisuleep commented Aug 1, 2023

I have read the CLA Document and I hereby sign the CLA

@Rexogamer Rexogamer requested review from insertish and removed request for insertish September 21, 2023 22:24
Copy link

@amycatgirl amycatgirl left a comment

Choose a reason for hiding this comment

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

I've left some suggestions regarding types in some functions in ServerIdentity.tsx

Other than that, LGTM

const [avatarId, setAvatarId] = createSignal(null);
const [uploading, setUploading] = createSignal<boolean>();

async function uploadToAutumn(file) {

Choose a reason for hiding this comment

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

You should use File | Blob instead of not providing a type (aka. implicit any) since the FileList type is just an array of Files (See MDN's article on FileList: https://developer.mozilla.org/en-US/docs/Web/API/FileList)

Suggested change
async function uploadToAutumn(file) {
async function uploadToAutumn(file) {


const handleFileSelect = async (event: Event) => {
const file = event.target?.files[0];
const json = await uploadToAutumn(file);

Choose a reason for hiding this comment

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

The target property in Event isn't guaranteed to be HTMLInputElement, so typescript doesn't know whether the files property actually exists or not and throws a error.

Suggested change
const json = await uploadToAutumn(file);
const handleFileSelect = async (event: Event & {target: HTMLInputElement}) => {


const handleFileSelect = async (event: Event) => {
const file = event.target?.files[0];
const json = await uploadToAutumn(file);

Choose a reason for hiding this comment

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

Suggested change
const json = await uploadToAutumn(file);
const handleFileSelect = async (event: Event & {target: HTMLInputElement}) => {


const handleFileSelect = async (event: Event) => {
const file = event.target?.files[0];
const json = await uploadToAutumn(file);

Choose a reason for hiding this comment

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

Typescript says that files can be null, so it's better to use the non-null assertion operator to tell it to shut up.

Suggested change
const json = await uploadToAutumn(file);
const file = event.target?.files![0];


const handleFileSelect = async (event: Event) => {
const file = event.target?.files[0];
const json = await uploadToAutumn(file);

Choose a reason for hiding this comment

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

Suggested change
const json = await uploadToAutumn(file);
const file = event.target.files![0];

@insertish insertish marked this pull request as draft October 13, 2023 16:55
@insertish
Copy link
Member

PR went stale, feedback not actioned / responded to, closing out for now
NB. conflicts with draft ServerIdentity coming in from M3 branch

@insertish insertish closed this Dec 1, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants