-
Notifications
You must be signed in to change notification settings - Fork 22
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(launchpad): Add Collection Creation form front #1477
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for teritori-dapp ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for gno-dapp ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
ce9dd57
to
2bf16bf
Compare
2bf16bf
to
6ca68e0
Compare
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.
LGTM
const hasErrors = (stepKey: number) => { | ||
if ( | ||
(stepKey === 1 && | ||
(!!collectionForm.getFieldState("name").error || | ||
!!collectionForm.getFieldState("description").error || | ||
!!collectionForm.getFieldState("symbol").error)) || | ||
!!collectionForm.getFieldState("coverImage").error || | ||
!!collectionForm.getFieldState("assetsMetadatas.nftApiKey").error | ||
) { | ||
return true; | ||
} | ||
if ( | ||
stepKey === 2 && | ||
(!!collectionForm.getFieldState("websiteLink").error || | ||
!!collectionForm.getFieldState("isDerivativeProject").error || | ||
!!collectionForm.getFieldState("projectTypes").error || | ||
!!collectionForm.getFieldState("isPreviouslyApplied").error || | ||
!!collectionForm.getFieldState("email").error) | ||
) { | ||
return true; | ||
} | ||
if ( | ||
stepKey === 3 && | ||
(!!collectionForm.getFieldState("teamDescription").error || | ||
!!collectionForm.getFieldState("partnersDescription").error || | ||
!!collectionForm.getFieldState("investDescription").error || | ||
!!collectionForm.getFieldState("investLink").error) | ||
) { | ||
return true; | ||
} | ||
if ( | ||
stepKey === 4 && | ||
(!!collectionForm.getFieldState("artworkDescription").error || | ||
!!collectionForm.getFieldState("isReadyForMint").error || | ||
!!collectionForm.getFieldState("isDox").error || | ||
!!collectionForm.getFieldState("daoWhitelistCount").error || | ||
!!collectionForm.getFieldState("escrowMintProceedsPeriod").error) | ||
) { | ||
return true; | ||
} | ||
if ( | ||
stepKey === 5 && | ||
(!!collectionForm.getFieldState("mintPeriods").error || | ||
!!collectionForm.getFieldState("royaltyAddress").error || | ||
!!collectionForm.getFieldState("royaltyPercentage").error) | ||
) { | ||
return true; | ||
} | ||
if ( | ||
stepKey === 6 && | ||
!!collectionForm.getFieldState("assetsMetadatas").error | ||
) { | ||
return true; | ||
} | ||
}; |
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.
Do react hook form not give some methods to handle this cleaner ?
Or should we do a map[step]list of field key to just loop through the fields corresponding to the steps
like for_, val := map[step] { if !! colectionForm.getFieldState(val).error} kinda
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.
In the return of useForm
hook you have an object named formState
and inside of it you have errors
. I don't know what infos you have exactly but can be a solution.
const { formState: { errors } } = useForm()
But agree with @MikaelVallenet that this if forest is not very clean, and if you can't handle this with errors
the map can be a better solution too
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.
The correct way could be to just control the formState.errors
yes.
I don't remember why it didn't work, but I'm checking and I'll fix.
Yes, it's messy and we need to add/remove rows from here after add/remove a field from the form...
I didn't enhanced very much this part (Stepper
), because thought about this issue (Creating a reusable form wizard): #1475
I'll just fix this comment for now
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.
Fixed: c85f0f5
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.
In general could you please use fontRegular
instead of fontMedium
or fontSemiBold
? I can do it if you want 👍
About using useFormContext
i think my mind can change because when you call it, you specify the type of the form so i'm not confusing when you call it, and found it better to read, it's not a lot of additional abstraction.
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 component is used once and it's almost the same than MultipleSelectInput
maybe you can use MultipleSelectInput
instead of create an other one ?
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 don't think so.
NetworkSelectorWithLabel
uses NetworkSelectorMenu
and its onPressNetwork
and filter
logic.
NetworkSelectorWithLabel
is closer to NetworkSelectorMenu
than MultipleSelectInput
an its dropdownOptions
const hasErrors = (stepKey: number) => { | ||
if ( | ||
(stepKey === 1 && | ||
(!!collectionForm.getFieldState("name").error || | ||
!!collectionForm.getFieldState("description").error || | ||
!!collectionForm.getFieldState("symbol").error)) || | ||
!!collectionForm.getFieldState("coverImage").error || | ||
!!collectionForm.getFieldState("assetsMetadatas.nftApiKey").error | ||
) { | ||
return true; | ||
} | ||
if ( | ||
stepKey === 2 && | ||
(!!collectionForm.getFieldState("websiteLink").error || | ||
!!collectionForm.getFieldState("isDerivativeProject").error || | ||
!!collectionForm.getFieldState("projectTypes").error || | ||
!!collectionForm.getFieldState("isPreviouslyApplied").error || | ||
!!collectionForm.getFieldState("email").error) | ||
) { | ||
return true; | ||
} | ||
if ( | ||
stepKey === 3 && | ||
(!!collectionForm.getFieldState("teamDescription").error || | ||
!!collectionForm.getFieldState("partnersDescription").error || | ||
!!collectionForm.getFieldState("investDescription").error || | ||
!!collectionForm.getFieldState("investLink").error) | ||
) { | ||
return true; | ||
} | ||
if ( | ||
stepKey === 4 && | ||
(!!collectionForm.getFieldState("artworkDescription").error || | ||
!!collectionForm.getFieldState("isReadyForMint").error || | ||
!!collectionForm.getFieldState("isDox").error || | ||
!!collectionForm.getFieldState("daoWhitelistCount").error || | ||
!!collectionForm.getFieldState("escrowMintProceedsPeriod").error) | ||
) { | ||
return true; | ||
} | ||
if ( | ||
stepKey === 5 && | ||
(!!collectionForm.getFieldState("mintPeriods").error || | ||
!!collectionForm.getFieldState("royaltyAddress").error || | ||
!!collectionForm.getFieldState("royaltyPercentage").error) | ||
) { | ||
return true; | ||
} | ||
if ( | ||
stepKey === 6 && | ||
!!collectionForm.getFieldState("assetsMetadatas").error | ||
) { | ||
return true; | ||
} | ||
}; |
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.
In the return of useForm
hook you have an object named formState
and inside of it you have errors
. I don't know what infos you have exactly but can be a solution.
const { formState: { errors } } = useForm()
But agree with @MikaelVallenet that this if forest is not very clean, and if you can't handle this with errors
the map can be a better solution too
const { width: windowWidth } = useWindowDimensions(); | ||
const scrollViewRef = useRef<ScrollView>(null); | ||
const isMobile = useIsMobile(); | ||
const collectionForm = useFormContext<CollectionFormValues>(); |
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.
In my refacto on react-hook-form, with norman we said to each other that don't use useFormContext
to handle our form. I don't have final opinion actually on how to use it, but i just want to be consistent when we use it
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.
Idk.
I just mention this issue here, because we are in LaunchpadStepper
scope: #1475 (Form wizard)
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.
Found this file too big.. Can't extract functions ?
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 strongly commented the code because it contains the mapping files warning/errors detection.. Maybe I can extract this logic yes
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 didn't figure out how to split this component...
If I add more components or files with functions, it will add more props and files juggling and adds difficulty to read.
I think it's well organized and clear when you fold the rows in IDE :/
Do you ave advises ?
(I still exported ResetAllButton
to another file: cc921e8)
style={[ | ||
fontMedium14, | ||
{ | ||
color: neutral77, | ||
}, | ||
]} |
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.
Can fit in one line
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.
As in a ton of lines in our code.
I think you should not take attention to that until we find a rule (lint + CI ?)
It could depend on the IDEs.
But we can lint by adding these lines in .eslintrc.js (These rules seem to work well):
"array-bracket-newline": ["error", "consistent"],
"object-curly-newline": ["error", "never"]
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.
Yeah there is a lot of lines.
Okay cool maybe we could add that in a specific PR ?
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.
Issue created: #1494
fontMedium14, | ||
{ | ||
color: primaryColor, | ||
}, | ||
]} |
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.
Same here
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.
Same here you can't use MultipleSelectInput
?
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.
Actually, no, it's nor the same usage: MultipleSelectInput
has Checkbox
in a deep level, it's "multiple".
But...
We want to refacto this MultipleSelectInput, and add common parent(s) used in SelectInputLaunchpad and MultipleSelectInput (And extract them from NFT Launchpad, and use them in ERC20 Launchpad, etc),
But
Should we do that in this PR?
@clegirar Your mind, but not fully about |
I'm not too fond of the FormProvider because it goes against composability. What's the point of making components if they can only be used in a specific context. Contexts are good for global data but I don't see the point of using them locally |
My main arg is just to have less props, I'm ok with yours. |
Only in Firefox, wtf Fixed: b01448e |
Done 4241965 |
I don't have final opinions on react-hook-form, actually the only thing i found cool it's the |
Honestly, I'm really ok with removing |
… child's vertical center on Firefox
more description could come soon...
Testable here: https://deploy-preview-1477--teritori-dapp.netlify.app/launchpad/apply (Teritori Testnet)
What's in this PR?
This PR is extracted from this one: #1024
It adds the Collection Creation form
The flow starts here:
<OmniLink
noHoverEffect
to={{ screen: "LaunchpadCreate" }}
style={{
flex: 1,
marginHorizontal: width >= MD_BREAKPOINT ? layout.spacing_x1_5 : 0,
marginVertical: width >= MD_BREAKPOINT ? 0 : layout.spacing_x1_5,
}}
>
<LargeBoxButton {...BUTTONS[1]} />
</OmniLink>
teritori-dapp/packages/screens/Launchpad/LaunchpadApply/LaunchpadApplyScreen.tsx
Lines 79 to 89 in a8d6ccd
The flow ends here:
await nftLaunchpadContractClient.submitCollection({
collection,
});
teritori-dapp/packages/hooks/launchpad/useCreateCollection.ts
Lines 182 to 184 in 3344964
I made this Zod object to pilot the Collection Creation form
teritori-dapp/packages/utils/types/launchpad.ts
Lines 80 to 153 in a8d6ccd
The data will be send onchain using the nft-launchpad contract's
submit_collection
:teritori-dapp/rust/cw-contracts/nft-launchpad/src/contract.rs
Line 83 in 3c3a39e
About Assets & Metadata
Use these files to test the step 6: test-files-step-6.zip
You will get these 2 warnings. It's expected, I wrongly filled the files to have these warnings as a demo.
So, you will get 3 images instead of 4 at the end.
You can fix the CSV files if you want, add assets, break the files, etc
The warnings don't block the flow, it just ignores the wrong assets.
The errors block the flow and ask the user to provide correct mapping files:
All these behaviors are handled here: https://github.com/TERITORI/teritori-dapp/blob/6ca68e031a528902d29ea9d559b466175de57902/packages/screens/Launchpad/LaunchpadApply/LaunchpadCreate/components/steps/LaunchpadAssetsAndMetadata/AssetsTab.tsx
I tried to well comment the code
What after this PR?
I want to separate the front in parts
Possible enhancement that can be done in another PRs
label
prop toCheckbox
.Related to chore(jsx-component): Enhance Checkbox #1481
MultipleSelectInput
reusable (Extract it from launchpad scope).Related to chore(jsx-component): Make a MultipleSelectInput #1482
NumberInput
. For now, we type numbers as string in forms and use a text input..Related to chore(jsx-component): Make a Number input #1483
CurrencyInput
(Extract it from launchpad scope).Related to chore(jsx-component): Make a CurrencyInput #1484
Organizations/comonents/RightSection
reusable (Create a reusable form wizard)And use it in these forms: Launchpad Create, DAO Creation, Multisig Creation, ... Everytime we have a form with steps
Related to chore(jsx-component): Make a form wizard #1475
We have to write a documentation
Especialy to guide the user on this step:
We must also provide two CSV templates: Attributes mapping file and an Assets mapping file