-
Notifications
You must be signed in to change notification settings - Fork 7
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
Fix DAO name doesn't update after edit #1530
Fix DAO name doesn't update after edit #1530
Conversation
✅ Deploy Preview for fractal-dev ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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 like you left in code that fetches cached DAO name from local storage. Can we remove that, too?
@adamgall ah yes, thanks for catching that. All done |
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 would encourage you to keep going even deeper.
Does the logic in useDAOName
make sense?
Do all the code paths get hit?
What are the actual differences between useDAOName
and useLazyDAOName
? Can we consolidate those functions into one thing?
Keep in mind my questions here are not rhetorical, I don't know the answers to them, I'm just superficially looking at the code and doing some searches for where these hooks/functions are being used and it seems ripe for some cleanup.
Ah yeah I'm glad you brought this up, because I had similar questions too but focusing on understanding and fixing both code and bug ended up sidelining it, and I forgot. I'd rather this potential cleanup not block this PR though. I would like to spend some time testing and working out what the hooks do differently, if they're both needed, and what a consolidated single hook might look like, but in a follow up PR. |
From a quick look at our leading DAOs, everything seems to work as expected - no issue from my side |
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.
Awesome, LGTM!
Thanks for the reviews guys. New issue created for the refactoring here: #1531 (comment) |
This PR removes the local storage of the DAO name. Since this data apparently exists on a subgraph, it's auto-updated (after some time).
There isn't much of a tradeoff in completely removing the DAO name cache. There was a split second where, in the breadcrumbs, the name would be empty while being fetched. I have replaced this with the DAO address substring, so it's less jarring.