-
Notifications
You must be signed in to change notification settings - Fork 51
Conversation
6c05120
to
c362208
Compare
Make sure to update to the latest anchor icon. The wrong one was in the icons file, my bad. |
@brandonhaslegs replaced both: cabe53a. |
ui/src/org.ts
Outdated
@@ -0,0 +1,342 @@ | |||
import * as svelteStore from "svelte/store"; |
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.
On the organization of code in this module and theGraphApi
I’d propose the following: Let’s split it into a stateless data fetching layer and the stateful “service” layer.
The data fetching layer would not contain any stateful code. It would not make use of stores, notifcations, module-level variables, etc. It’s primary purpose would be to export functions that get or update org-related data from the sources. (Maybe we separate data fetchers for the graph, contracts, and gnosis safe.)
On top of this layer would be the layer that keeps state and interacts with the UI (e.g. through notifications.)
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.
ui/Modal/Org/AnchorProject.svelte
Outdated
let projectUrn: urn.Urn | undefined = undefined; | ||
|
||
$: if (projectUrn) { | ||
projectScreen.fetch(projectUrn); |
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 fear it will be quite dangerous re-using and mutating the global project screen store here.-We should use a dedicated project store only for this view. I don’t know however how much of the logic of the screen store we would need to reuse to accomplish this. To be able to re-use logic may need to better separate the data fetching logic from the state update logic in the project module.
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.
Tracked in #1998.
], | ||
}); | ||
|
||
router.push({ type: "org", address: orgAddress, activeTab: "projects" }); |
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.
router.push
is async now. Is there a good reason not to await?
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.
What do we gain by await
ing here? There are no actions that happen after. We also don't await
any of the other router.push
es anywhere else in the code.
Some feedback from Abbey and my review:
|
@brandonhaslegs we'll have to figure out how to make sure that the wallet in the browser is connected to the same Ethereum network as the app. Or instruct the user to match the network settings. Otherwise if the user clicks on "Open in browser" and the networks mismatch, they will see: |
Updated the icons so we don't have two identical icons in the same dropdown. Use the |
Hi, here's a |
I think this is fine for now. Most likely that they will connect to the same network. |
I also think this won't be a problem once we implement: #2002. |
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.
A couple of final comments
Co-authored-by: Julien Donck <[email protected]> Co-authored-by: Thomas Scholtes <[email protected]> Co-authored-by: Alexis Sellier <[email protected]> Co-authored-by: Rūdolfs Ošiņš <[email protected]> Signed-off-by: Rūdolfs Ošiņš <[email protected]>
This PR implements radicle Orgs on Ethereum via Gnosis Safe.
All tasks are outlined in the "Orgs" section of: #1893.