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(PE-7068): reassign name #624

Merged
merged 10 commits into from
Dec 18, 2024
Merged

feat(PE-7068): reassign name #624

merged 10 commits into from
Dec 18, 2024

Conversation

atticusofsparta
Copy link
Contributor

No description provided.

@atticusofsparta atticusofsparta requested a review from a team as a code owner December 13, 2024 16:36
Copy link

github-actions bot commented Dec 13, 2024

Visit the preview URL for this PR (updated for commit 92fc632):

https://arns-portal--pr624-pe-7068-reassign-nam-304byfoo.web.app

(expires Wed, 01 Jan 2025 19:33:12 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 1f59769442cb89cf2a92042a342e51fe94047b94

}
}

if (!show) return <></>;

Choose a reason for hiding this comment

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

nit: could have this at the top of the component to prevent the above code from running

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hooks need to be mounted deterministically

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That being said, because this uses the useDomainInfo hook, it shouldnt be doing anything extra here, since it is being access from a page that is also using that hook - internally it uses useQuery which will dedupe the request and return the same data to all the components calling it.

Comment on lines +184 to +195
<button
className="bg-primary-thin p-3 rounded"
onClick={() => setWorkflow(REASSIGN_NAME_WORKFLOWS.NEW_EXISTING)}
>
Create new ANT
</button>
<button
className="bg-primary-thin p-3 rounded"
onClick={() => setWorkflow(REASSIGN_NAME_WORKFLOWS.EXISTING)}
>
Use existing ANT
</button>

Choose a reason for hiding this comment

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

nit: common <button>s could share a component to share css styling

<div className="modal-container">
<div className="flex flex-col rounded-md border border-dark-grey bg-foreground p-6 w-[32rem] gap-8 text-white">
<div className="flex flex-row justify-between items-center">
<h2 className="text-[24px]">Name Reassignment</h2>
Copy link

@fedellen fedellen Dec 13, 2024

Choose a reason for hiding this comment

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

does text-[24px] become responsive by override elsewhere? I would expect a callout to a text variable that handles different screen widths

Choose a reason for hiding this comment

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

especially since they're hard coded throughout. getting each view right for each screen widths would prove more challenging without some shared responsiveness in the app

Copy link

@fedellen fedellen Dec 13, 2024

Choose a reason for hiding this comment

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

not necessary to do any of this in this PR's context, just calling out some maintainability nits

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, i should be using rem here

Comment on lines +541 to +559
'size-[25px] cursor-pointer rounded-full border border-white shadow-[0_2px_10px] shadow-black outline-none hover:bg-white focus:shadow-[0_0_0_2px] focus:shadow-black transition-all',
},
{
label: (
<label
htmlFor="r1"
className={`pl-[15px] text-[15px] leading-none ${
workflow === REASSIGN_NAME_WORKFLOWS.NEW_BLANK
? 'text-white'
: 'text-grey'
} whitespace-nowrap cursor-pointer hover:text-white`}
>
Use blank slate
</label>
),
value: REASSIGN_NAME_WORKFLOWS.NEW_BLANK,
className:
'size-[25px] cursor-pointer rounded-full border border-white shadow-[0_2px_10px] shadow-black outline-none hover:bg-white focus:shadow-[0_0_0_2px] focus:shadow-black transition-all',
},
Copy link

@fedellen fedellen Dec 13, 2024

Choose a reason for hiding this comment

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

nit: repeated styles, could share a simple var so that when we make changes to styles, we make them consistently

const radioGroupReassignNameStyles: string = `my-styles`

or go more fancy, depending on how widespread styles are repeated

const radioGroupReassignNameStyles: string = `${commonStyles} ${radioGroupStyles} ${reassignNameStyles}`

Comment on lines +164 to +172
// for UX so the user sees the signing message
await sleep(2000);
result = await antProcess.reassignName({
name: payload.name,
arioProcessId: payload.arioProcessId,
antProcessId: newAntProcessId,
});
break;
}

Choose a reason for hiding this comment

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

could explain further on the comment here. Also, await spawnANT should not end until that message is already signed

Copy link

@fedellen fedellen Dec 13, 2024

Choose a reason for hiding this comment

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

ah, I get it -- its so the user can see the 'Reassigning ArNS name, please wait... ' + message

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly yeah - noticed the reassign was a bit quick sometimes and it didn't even give the message time to rerender.

fedellen
fedellen previously approved these changes Dec 13, 2024
Copy link

@fedellen fedellen left a comment

Choose a reason for hiding this comment

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

left a few nits and questions, looking good!

});
}}
>
<Star className={`w-[16px]`} />{' '}
Copy link
Contributor

Choose a reason for hiding this comment

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

use rem

kunstmusik
kunstmusik previously approved these changes Dec 16, 2024
@atticusofsparta atticusofsparta merged commit e6feb31 into develop Dec 18, 2024
6 checks passed
@atticusofsparta atticusofsparta deleted the PE-7068-reassign-name branch December 18, 2024 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants