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.

Sign up for GitHub

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

Jump to bottom

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

Closed

aisuleep wants to merge 2 commits into revoltchat:master from aisuleep:aisu/feat/server-identity

aisuleep commented Jul 30, 2023

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


          feat(modal): port ServerIdentity revoltchat#112

8a5c626

github-actions bot commented Jul 30, 2023 •

edited

Loading

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

Author

aisuleep commented Jul 30, 2023

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. 👍

Author

aisuleep commented Aug 1, 2023

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


          fix: show cursor pointer on remove button hover

Rexogamer requested review from insertish and removed request for insertish

September 21, 2023 22:24

amycatgirl suggested changes

View reviewed changes

amycatgirl left a comment

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

Other than that, LGTM

components/modal/modals/ServerIdentity.tsx

+              const [avatarId, setAvatarId] = createSignal(null);
+              const [uploading, setUploading] = createSignal<boolean>();
+              async function uploadToAutumn(file) {

amycatgirl Sep 22, 2023

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) {

components/modal/modals/ServerIdentity.tsx

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

amycatgirl Sep 22, 2023

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}) => {

components/modal/modals/ServerIdentity.tsx

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

amycatgirl Sep 22, 2023

Suggested change

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

components/modal/modals/ServerIdentity.tsx

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

amycatgirl Sep 22, 2023

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];

components/modal/modals/ServerIdentity.tsx

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

amycatgirl Sep 22, 2023

Suggested change

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

insertish marked this pull request as draft

October 13, 2023 16:55

Member

insertish commented Dec 1, 2023

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

insertish closed this

github-actions bot locked and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet