-
Notifications
You must be signed in to change notification settings - Fork 408
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: Safe version/upgrade button improvements #1627
Conversation
@iamacook if the article's ready, lemme know. I'll link that. |
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.
Thank you for your contribution, @abheektripathy - it looks very cool so far!
I have a few small suggestions.
Co-authored-by: Aaron Cook <aaron@safe.global>
Co-authored-by: Aaron Cook <aaron@safe.global>
Co-authored-by: Aaron Cook <aaron@safe.global>
@iamacook thanks for the review, i'll keep in mind to follow this code-style in the future. |
Since an env is not created for these kind of tickets I ask Aaron to show it to me running it locally. We did a quick edit on the type of icon, because the original one (a square with a check) looks like a clickable checkbox: Note: there is a task about the notification that should not show up more than once. That task will be done in another ticket. |
Thanks once again for the contribution, @abheektripathy!
If you'd like to tackle that as well @abheektripathy, you're more than welcome to! I think it would make sense in a separate PR.
|
Thanks @abheektripathy! |
What it solves
Resolves #1530
Tasks-
How this PR fixes it
Added a conditional that checks the
Safe's
state, if it'sUP_TO_DATE
shows Latest Version or else a link with the tooltip as mentioned.How to test it
go to the settings page to verify your safe version.
Analytics changes
Screenshots
(for this screenshot i hardcoded the safe version to be
OUTDATED
)