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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat: Better complex struct inputs #702

Merged
merged 28 commits into from Feb 14, 2024
Merged

Feat: Better complex struct inputs #702

merged 28 commits into from Feb 14, 2024

Conversation

technophile-04
Copy link
Collaborator

@technophile-04 technophile-04 commented Jan 30, 2024

Description

Screen.Recording.2024-02-06.at.3.28.11.AM.mov

Tried handling better structs inputs and complex structs array, lol not the best optimized approach but in my testing seems to work nicely

For testing I have updated YourContract.sol with a nested struct along with a multidimensional struct array but please feel free to play around / test different cases.

You could also try putting Opensea contract in externalContract on mainnet to see how UI looks for testing.

Didn't go too much on visual UI part but maybe we could handle it more better just wanted to get functionality inplace.

Also note, this PR only handles structs array and not primitive types array maybe we could it another PR since it should be alot easier and didn't wanted to clutter this PR 馃檶

OG Idea: https://twitter.com/_jxom/status/1748068858047668227

@technophile-04 technophile-04 marked this pull request as draft January 30, 2024 09:02
@technophile-04 technophile-04 marked this pull request as ready for review February 6, 2024 09:58
@rin-st
Copy link
Collaborator

rin-st commented Feb 9, 2024

Magic! 馃獎
Looks awesome! Really great job!

Noticed that I can't send array with 0 entries, there's at least one and I can't remove it.
Also a question: do we need it also for reading, or only for writing?

@technophile-04
Copy link
Collaborator Author

Noticed that I can't send array with 0 entries, there's at least one and I can't remove it.

Ohh yeah, I completely forgot that you could pass in empty array fixed at df3fd3e, by default it shows 0th element (I think its nice UX to show it at default, but we could keep empty by default) but you can remove 0th element and empty array will passed

Also a question: do we need it also for reading, or only for writing?

Yeah lol again forgot about read ! fixed at 1394397 also updated test contract

Thanks Rinat 馃檶

Copy link
Collaborator

@rin-st rin-st left a comment

Choose a reason for hiding this comment

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

Awesome, thanks!

Copy link
Collaborator

@Pabl0cks Pabl0cks left a comment

Choose a reason for hiding this comment

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

This is just GREAT!! Awesome job implementing it @technophile-04 馃敟馃敟

I like UI a lot! The only thing I felt that could be a bit confusing to some users was the visual representation for each tuple item (0, 1, 2..). Maybe we could add a bit more descriptive text like Item[0], or TupleItem[0] or something similar.

If we want to add more visual differentiation between tuple items we could play with bg color. Here is an example with zebra pattern, but maybe that's a bit ugly, this could go in a future iteration with some designer help :)

image

@technophile-04
Copy link
Collaborator Author

visual representation for each tuple item (0, 1, 2..). Maybe we could add a bit more descriptive text like Item[0], or TupleItem[0] or something similar.

Yup, completely get it, updated at cc89cfe

Screenshot 2024-02-13 at 11 28 13鈥疨M

  • Used tuple instead item or any other name since people can name the args as item or any other thing and it will clash with our naming and might make it more confusing (they won't be able to use tuple as arg name since its reserved)
  • Only showing tuple[index] for 1D array elements since technically marking 2D / ND marking their all levels with tuple[index] might not make sense

to add more visual differentiation between tuple items we could play with bg color. Here is an example with a zebra pattern, but maybe that's a bit ugly, this could go in a future iteration with some designer help

Love this idea !!!! It kind of makes it visually easy to digest, we can think maybe changing the color a bit for an alternate zebra strip, not adding this changes in PR but should probably handle this in separate PR 馃檶

Tysm Pablo for the great feedback !!

@technophile-04
Copy link
Collaborator Author

Tysm all for testing !! Just locked dasiyUI version at f9c4a0f since they seem to have broken Accordian in their latest version and when you deploy to prod we use their version and its broken there eg:

Screenshot 2024-02-14 at 6 56 45鈥疨M

The accordian arrow does not show up here https://se2-structs.vercel.app/

After locking daisyUI version here is prod version : https://se2strcut-prod.vercel.app/

TODO:

  1. Create an issue on SE-2 for better styling of this Feat: Better complex struct inputs聽#702 (review)
  2. Create an issue on DaisyUI regarding Accordian issue

Merging this 馃檶

@technophile-04 technophile-04 merged commit a5eedbd into main Feb 14, 2024
1 check passed
@technophile-04 technophile-04 deleted the feat/better-structs branch February 14, 2024 13:41
@github-actions github-actions bot mentioned this pull request Feb 15, 2024
kmjones1979 added a commit that referenced this pull request Feb 15, 2024
* Add Optimism Sepolia config (#711)

* Use arbitrumSepolia instead of Goerli (#716)

* up rainbowkit version to 1.3.5 (#719)

* removing lock file

* use next-themes to handle theme and update usehooks-ts (#707)

* Feat: Better complex struct inputs (#702)

* improve debug struct UI (#726)

* add basic example to show connected address (#721)

---------

Co-authored-by: winnsterx <46658657+winnsterx@users.noreply.github.com>
Co-authored-by: Shiv Bhonde | shivbhonde.eth <shivbhonde04@gmail.com>
Co-authored-by: Carlos S谩nchez <oceanrdn@gmail.com>
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.

None yet

3 participants