-
Notifications
You must be signed in to change notification settings - Fork 18
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
feat: move edit profile, delegations and treasuries into settings page #653
Conversation
23d657d
to
94e0fd2
Compare
Profile page should not have header with title and description and should have a max width, maybe you seen previous version of the design, here is the design: https://www.figma.com/design/hmHTau1Y1KYzrBBKLQr5QV/%23design?node-id=5248-3827&t=wT7znFIbQqTY8ivx-0 Others pages should also have a fixed max width and not take the whole space I found also these issues earlier: https://github.com/snapshot-labs/workflow/issues/178 |
I didn't use any designs there, because those weren't linked in the issue so I just copied those as-is 😆 Didn't know those designs are final as they looked WIP (profile page has things we don't support and somehow has authenticators included as well, delegations support only single delegation). Is delegation also supposed to updated to this new design (that supports only single delegation)? |
Yes my bad there are still some part that is in progress, I've removed the authenticators this must be a mistake
No, we are moving away from this see https://github.com/snapshot-labs/workflow/issues/177 |
Missed it in the previous PR, but shouldn't the navigation be sticky ? |
Error state on the inputs are not persisted between tab navigation:
|
The bottom bar with the reset button and error message does not disappear in some case:
|
Good catch, it should be sticky indeed. |
May need adjust the gradient background to be less transparent below the arrow, to make it pop, and make the border between the arrow button and the menu below more clear. Right now, when clicking in this area, we're not sure if we're clicking on the nav menu below, or the > button And not sure about if these < and > buttons should be focusable on keyboard navigation, as the tab will already scroll the element into view |
And not related to this PR itself, but clicking on the picker in the Delegation contract address form does not do anything, when adding a governor delegation with contract address |
Addressed all comments, I haven't fully matched profile page to designs at the moment as I'm not sure if we want to do it in this PR and if it's ready. |
@Sekhmet No we don't need to address this for the moment. I see a small issue that move the profile page up when I click on another tab. |
For some reason scrollBy might not scroll all the way (for example 199.5 instead of 200). Further calls won't cause another scroll so we might be missing 0.5 to the right. This changes it so if we are off by less than 1 pixel we still treat it as scrolled all the way to the right.
More general solution is needed: #573
8e04095
to
7f6b851
Compare
This somehow prevents vertical scroll that causes profile page to scroll away.
@bonustrack it's weird bug related to scrolling element into view, but it should be fixed now. |
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.
tACK
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.
tAck, aside from the above 2 comments, which are not deal breaking, and can be fixed later
Summary
This PR removes edit profile/delegations/treasuries into settings page.
Closes: https://github.com/snapshot-labs/workflow/issues/166
Note: There is issue with EVM networks when saving networks as with TheGraph when updating metadata IPFS is not updated in sync with space itself so there is period where space is updated but IFPS wasn't resolved yet (and metadata is missing on space object). We reload space on save so it might be possible that you will end up with failure there, this would be resolved once we migrate from TheGraph.
How to test
Screenshots
Screen.Recording.2024-08-23.at.14.34.50.mov