-
Notifications
You must be signed in to change notification settings - Fork 9
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: project forms validation #1794
Conversation
@@ -0,0 +1 @@ | |||
export const DESCRIPTION_MAX_LENGTH = 1400; |
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.
@erikalogie I'm not sure what is the actual description max length we want, on the figma it's less than that but in the SHACL definitions, you set it to 1400 @wgwz
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.
@blushi I think that 1400 is probably fine if we have a "read more" link implemented on the front-end, which it looks to me like we don't have anymore (or maybe we only ever had it for the "additional story" section?). Otherwise maybe we should limit it to more like 1,000 characters.
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.
Ideally we would have the read more though
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.
@erikalogie looking at the code, we currently have the "read more" only for the land steward story but not for the generic description, I can create an issue to handle that
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 can't recall a reason that i set it to 1400, i probably was just copying from something that already existed in the standards. just wanted to mention that
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.
78c6901
to
0beb075
Compare
0beb075
to
9026562
Compare
@erikalogie also see testing instructions |
const metadata = getProjectCreateBaseData(creditClassOnChainId); | ||
setCreditClassId(creditClassOnChainId); | ||
const metadata = getProjectCreateBaseData(creditClassOnChainId); | ||
setCreditClassId(creditClassOnChainId); |
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 just wanted to make a note here that i would like to understand this part better, i'm having trouble visualizing what the actions in UI are that correspond to this code (choosing a credit class and why we update project). i'd like to understand this better. i'm just missing some context. not a blocker on this by any means
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.
choosing a credit class corresponds to this page: https://www.figma.com/file/TyhFaXqko9F8OvLEE4Zomg/Issuer-Create%2FEdit-Projects%2C-Credit-Class%2C-Batches?node-id=2%3A4630&t=gYtGMt1zelv99hxH-0, this is the very first form you get when creating a new project
we update the off-chain project to save the chosen credit class
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.
looks great! i think it was a good choice to redo this validation with yup (and eventually zod). it's simpler code, easier to maintain.
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
d2bb3cb
to
8b2ed24
Compare
@blushi is this what's expected? |
yes for C01 projects, the metadata should have those fields that are missing |
@blushi ok great, then this looks like it is working as expected |
Description
Closes: regen-network/rnd-dev-team#1501
ProjectMetadataForm
since the validation depends on the credit class so in this case, it uses SHACLFieldFormControl
[v2] Validation runs on old values after setFieldTouched jaredpalmer/formik#2083Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
redwood
,hambach
and optionallymaster
(see below)Setting up backport PRs
After merging your PR to
dev
, set up backports by doing the following:your PR:
@Mergifyio backport redwood hambach
.@Mergifyio backport redwood hambach master
.If your branch does have merge commits:
a. Pull latest
dev
,hambach
andredwood
branchesb. Create new branches for backports and merge
dev
(replace<PR#>
with your PR #)c. Open new PRs in regen-web targeting
hambach
andredwood
, respectively.d. If PR is a bug fix repeat the same process for
master
branch.How to test
Create a new project from https://deploy-preview-1794--regen-registry.netlify.app/ecocredits/projects (using our test addr regen1df675r9vnf7pdedn4sf26svdsem3ugavgxmy46) and go through the different forms to make sure the validation is working correctly.
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...