-
Notifications
You must be signed in to change notification settings - Fork 705
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
Notify app updates #1059
Notify app updates #1059
Conversation
<span className="upgrade-text">Upgrade</span> | ||
<ArrowUpCircle color="white" size={25} fill="#82C341" className="notification" /> | ||
</button> | ||
<span className="tooltiptext tooltip-top">New version ({updateVersion}) found!</span> |
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've removed this tooltip text because that info is already explained in the new info
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.
Sounds good to me. Do you think the tooltip feature may be used in another place of this app? We may want to keep it for other use cases. However, I see we're using it as an inline CSS style, so we will need to create a component. Definitively, it can be done in a separate task but I wanted to highlight it :)
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.
right, for the moment nothing would use it so we don't need it but it's true that moving that to a component can be helpful
takes a mental note
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.
LGTM @andresmgot ! I added a few comments but they're not blockers of this PR :)
<span className="upgrade-text">Upgrade</span> | ||
<ArrowUpCircle color="white" size={25} fill="#82C341" className="notification" /> | ||
</button> | ||
<span className="tooltiptext tooltip-top">New version ({updateVersion}) found!</span> |
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.
Sounds good to me. Do you think the tooltip feature may be used in another place of this app? We may want to keep it for other use cases. However, I see we're using it as an inline CSS style, so we will need to create a component. Definitively, it can be done in a separate task but I wanted to highlight it :)
text-align: center; | ||
padding: 0.25em 0; | ||
font-size: 14px; | ||
background: #008145; |
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.
Aren't we using HEx library in this project? I don't remember, honest. You can use the color
method if it's available.
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.
it seems we are not :)
return ( | ||
<div> | ||
updateContent = ( | ||
<React.Fragment> |
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.
Nice! :)
Fixes #1039