-
Notifications
You must be signed in to change notification settings - Fork 8
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
Reading/Writing Features #123
Conversation
…init respectively
…rom contract index in read functions
…rom contract index in write 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.
Here are some things to consider.
- No wallet connect support? Is this intentional or something we plan to introduce (since we use
@concordium/react-components
)? - As users can now do more stuff than deploy/init contracts, it would make sense to think about user flows (in my opinion)
- I would also introduce a router (react-router), as it lets you be more declarative about individual sections of the application, with the added benefit of persisting the location in the url along with associated state. This will allow users to use browser navigation (back/forward) to navigate the app + link to selected parts of the application, which might be useful.
Possible application routes (marked with bold)
- / (root = Deploy/declare module)
- /module/:module-reference
- Instantiate contract
- /module/:module-reference
- /instance
- Declare contract details (can we just always derive contract name and possible entrypoints if possible?)
- /instance/:index/read (do we need subindex?)
- /instance/:index/write (do we need subindex?)
I realize these changes are quite substantial, but will improve usability a lot.
|
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 pretty good overall.
I have some relatively minor comments.
There is one general thing about the UI, but also some details of the flows could be revised a bit.
But we don't have time to spend on fiddling with that at the moment. This can be part of the next iteration.
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 still think it's worthwhile to look into the structure of these inputs that trigger other changes (effects or derived values), which is my main issue with the code as it is right now.
I would prefer if we stuck with the recommendations from the documentation of the forms library, which revolve around
- spread
form.register
onto input - use
Controller
to integrate with controlled field components - use validation rules and associated error messages registered
- use
useWatch
to handle complicated derived values or effects
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 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 still some issues (see video shared in slack).
But if those are resolved I think we should merge this.
While I understand @soerenbf 's concerns, we also have to get this done.
We'll have to prioritize handling the backlog later, since we have other things we need to be working on as well. It's good there is an issue with all the concerns listed.
5be5c38
to
a17f5fb
Compare
2975400
to
4ef17b4
Compare
Purpose
closes #127
Changes