-
-
Notifications
You must be signed in to change notification settings - Fork 7
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/fetch cm from wallet and update cm #15
Conversation
Co-authored-by: Begoña Álvarez de la Cruz <balvarez@boxfish.studio>
components/CreateCM/Form.tsx
Outdated
storage: '', | ||
files: [], | ||
} as const; | ||
function isFormUpdateIsValid(): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double "is" is redundant, i think isFormUpdateValid
is better
hooks/useUploadCache.tsx
Outdated
Array.from(e.target.files).forEach((file) => { | ||
fileList.push(file); | ||
}); | ||
setCache(fileList[0]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If only the first value of the array is necessary, why is the whole array iterated?
components/CreateCM/Form.tsx
Outdated
|
||
const { files, uploadAssets } = useUploadFiles(); | ||
const { cache, uploadCache } = useUploadCache(); | ||
const [interactingWithCM, setInteractingWithCM] = useState(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This state stores a boolean, i think it's better that the name starts with is isInteractingWithCM
solTreasuryAccount: PublicKey; | ||
itemsRedeemed: BN; | ||
} | ||
|
||
export interface CandyMachineConfig { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion! I was checking firefly best practices page, and they name the interfaces with a capital "i" at the beginning. In this case it would be ICandyMachineConfig
If you see it correct, we could implement it for all interfaces
https://iotaledger.github.io/firefly/guides/coding-conventions/naming/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally prefer not using prefixes or suffixes for types/interfaces
pages/list-candy-machines/[id].tsx
Outdated
</Head> | ||
<div className='flex justify-center items-center flex-col'> | ||
<Title text='Update Candy Machine' /> | ||
<span className='mt-8'>{account}{" "}<a className='text-blue-700' href={`https://solscan.io/account/${account}?cluster=devnet`}>View in Solscan</a></span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think its better that the link is opened in another tab
<a className='text-blue-700' href={"https://solscan.io/account/${account}?cluster=devnet"} target="_blank" rel="noopener noreferrer">View in Solscan</a>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally agree.
@@ -0,0 +1,84 @@ | |||
import { BN } from '@project-serum/anchor'; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use moment.js to do many of the date functions
* @param time time to parse | ||
* @returns {string} time parsed to UCT | ||
*/ | ||
export function parseDateToUTC(dateTime: string, time: string): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moment has a function for this moment.utc
return `${hours}:${minutes}`; | ||
} | ||
|
||
export function getCurrentTime() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moment has a function for this moment().format('LT')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are 2 reasons why I created these functions manually:
- Reduce size of the project, since we are only using 5 silly functions to parse dates, I decided to just write them up myself. We are not doing heavy date manipulation, therefore I consider it is not needed.
- All these functions (moment.utc() and moment().format('LT')) do not provide the desired outcome: if the date is "8:00", the desired outcome should be "08:00".
According to the momentjs docs here the format("LT") provides "8:30 PM" but we would only need "08:30" (0 at the start and remove AM/PM).
For the UTC we need "2 May 2021 08:00:00 GMT", but the moment.utc() provides "2013-02-04T18:35:24+00:00" for example.
In this case I think we would not obtain any benefit by using moment.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
const { connection } = useConnection(); | ||
const [candyMachineConfig, setCandyMachineConfig] = | ||
useState<FetchedCandyMachineConfig>(); | ||
const [loading, setLoading] = useState({ loading: false, error: false }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this state checks if the loading spinner is shown, or an error has to be shown
If it does this, I think const [spinner, setSpinner] = useState({ loading: false, error: false });
better explains what this state does
View in Solscan | ||
</a> | ||
</span> | ||
{loading.loading && <Spinner />} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have commented the previous comment because this does not give me much information - loading.loading
pages/list-candy-machines/index.tsx
Outdated
CANDY_MACHINE_PROGRAM_V2_ID, | ||
{ | ||
commitment: 'confirmed', | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This space is not necessary
…rd into feat/fetch-cm-from-wallet-and-update-cm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job!
This PR includes:
Closes #9, #8