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

Create pool redesign - feature branch #2537

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open

Conversation

kattylucy
Copy link
Collaborator

@kattylucy kattylucy commented Nov 11, 2024

Copy link

github-actions bot commented Nov 11, 2024

PR deployed in Google Cloud
URL: https://app-pr2537.k-f.dev
Commit #: ee0c6d8
To access the functions directly check the corresponding deploy Action

Copy link

github-actions bot commented Nov 11, 2024

PR deployed in Google Cloud
URL: https://pr2537-app-ff-production.k-f.dev
Commit #: ee0c6d8
To access the functions directly check the corresponding deploy Action

@kattylucy kattylucy force-pushed the create_pool_redesign branch 4 times, most recently from c962a38 to 4d6c428 Compare November 15, 2024 11:18
@kattylucy kattylucy force-pushed the create_pool_redesign branch from c6811f7 to 45db2a9 Compare November 21, 2024 09:50
@kattylucy kattylucy force-pushed the create_pool_redesign branch from 45db2a9 to 19988cc Compare November 21, 2024 09:56
@kattylucy kattylucy changed the title Create pool redesign Create pool redesign - feature branch Nov 21, 2024
kattylucy and others added 4 commits November 21, 2024 11:03
* Add pool structure UI changes

* Small UI fix

* Avoid deletion on entries if less than one

* Add logic to single / multi sign

* Fix linter errors
* Fix ts error and change logic for onboarding values

* Add create pool existing functionality

* Cleanup types

* cleanup

* Add deposit banner

* Fix linter errors

* Add metadata values

* Cleanup types

* Add onboarding functionality and UI fixes

* Add proxies functionality

* Fix ts errors

* Add create pool dialog

* Add dialogs

* Add review feedback

* wip

* Add waiting before redirecting to avoid error

* Remove default empty pool fee
* Bug fixes and add proposal link

* Fix ratings creating empty value
@kattylucy kattylucy marked this pull request as ready for review December 4, 2024 11:45
Copy link
Collaborator

@sophialittlejohn sophialittlejohn left a comment

Choose a reason for hiding this comment

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

Amazing to see this come together so nicely 🔥

There are a few comments from me and a few from the github bot that are probably worth fixing.

Screenshot 2024-12-04 at 13 32 25
  • Here, I'm wondering what the wallet address is for this input? Would that be the pool admin?
  • If single is selected, I think we should hide the threshold section underneath.
  • The address (single and multisig) should be validated like the rest of the address inputs and show a red error message if it doesn't match
Screenshot 2024-12-04 at 13 38 54 Here I think we should clean up the look of the box when there is an error message ;)

Lastly, I tried to create a pool and it failed with this error message:

Error: createType(Call):: Call: failed decoding poolRegistry.register:: Struct: failed on args: {"admin":"AccountId32","pool_id":"u64","tranche_inputs":"Vec<PalletPoolSystemTranchesTrancheInput>","currency":"{\"_enum\":{\"Native\":\"Null\",\"Tranche\":\"(u64,[u8;16])\",\"__Unused2\":\"Null\",\"AUSD\":\"Null\",\"ForeignAsset\":\"u32\",\"Staking\":\"CfgTypesTokensStakingCurrency\",\"LocalAsset\":\"u32\"}}","max_reserve":"u128","metadata":"Option<Bytes>","write_off_policy":"Vec<PalletLoansPolicyWriteOffRule>","pool_fees":"Vec<(CfgTraitsFeePoolFeeBucket,CfgTypesPoolsPoolFeeInfo)>"}:: Struct: failed on pool_fees: Vec<(CfgTraitsFeePoolFeeBucket,CfgTypesPoolsPoolFeeInfo)>:: Tuple: failed on 1:: Struct: failed on feeType: {"_enum":{"Fixed":"{\"limit\":\"CfgTypesPoolsPoolFeeAmount\"}","ChargedUpTo":"{\"limit\":\"CfgTypesPoolsPoolFeeAmount\"}"}}:: Cannot map Enum JSON, unable to find '' in fixed, chargedupto

I think it should be an easy fix but I'm happy to help you debug tomorrow!

fabric/src/theme/tokens/colors.ts Outdated Show resolved Hide resolved
@@ -17,7 +17,7 @@ const statusPromoteBg = '#f8107114'
const colors = {
textPrimary: grayScale[800],
textSecondary: grayScale[500],
textDisabled: grayScale[300],
textDisabled: '#6A7280',
Copy link
Collaborator

Choose a reason for hiding this comment

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

would it make sense to adjust a value in the grayScale instead of defining a new one?

)

const ratings = values.poolRatings.map((rating, index) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

can be removed

form.setFieldValue(field.name, event.target.value)
}
return props.isUrl ? (
<URLInput
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems like the wrong place for this. Also don't think it's needed. You should be able to use the URLInput as a prop to FieldWithErrorMessage:

<FieldWithErrorMessage
  name="website"
  as={URLInput}
  label="Website URL"
  placeholder="www.example.com"
  validate={validate.websiteNotRequired()}
/>

const [, , , , poolId] = args
if (createType === 'immediate') {
setCreatedPoolId(poolId)
navigate(`/pools/${poolId}`)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should stay in the useEffect like it was before. The data for the newly created pool won't always be immediately loaded, so it should wait for the data of the new pool before navigating to it.

@@ -208,6 +208,7 @@ function AOForm({
}),
newMetadata ? cent.pools.setMetadata([poolId, newMetadata], { batch: true }) : of(null),
]).pipe(
// THIS IS WHERE PROXIES ARE BEING CREATED
Copy link
Collaborator

Choose a reason for hiding this comment

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

can be removed

label,
errorMessage,
extendedClickArea,
variant = 'round',
Copy link
Collaborator

@onnovisser onnovisser Dec 10, 2024

Choose a reason for hiding this comment

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

This doesn't make sense, checkboxes should always be square. For single choice inputs, you can use RadioButton

@kattylucy kattylucy requested a review from onnovisser December 10, 2024 14:39
@onnovisser
Copy link
Collaborator

Looking nice Katty! Just a few things I noticed

  • This button is misaligned when there's an error
    image

  • Checkbox looks a bit misaligned too
    image

  • Should there be a way to remove an address input here?
    image

  • There's an ImageUpload component that may be better used for uploading images, instead of the FileUpload
    image
    image
    The changed and currently used FileUpload component doesn't give a preview
    image

  • Also the FileUpload component used to have a button to remove the file, which would be good to keep I think, because they aren't always required fields.
    Now
    image
    Before
    image

  • These placeholders have a different color
    image

@kattylucy kattylucy force-pushed the create_pool_redesign branch from 2f39697 to 17d3fde Compare December 12, 2024 09:42
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