-
-
Notifications
You must be signed in to change notification settings - Fork 8
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
WEB-3418 - Use Redirect instead of Pop-up when connecting Provider on Mobile #447
Conversation
import InfoIcon from './assets/info-outline-24-px.svg'; | ||
import CollapseIconOpen from './assets/expand-more-24-px.png'; | ||
import CollapseIconClose from './assets/chevron-right-24-px.png'; | ||
import InfoIcon from './assets/info-outline-24-px.png'; |
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.
See this comment: https://github.com/tidepool-org/blip/pull/1540/files#r2010755798
This is the "before" state after a redirect: https://github.com/user-attachments/assets/f6d02164-da07-4470-ae02-acf4328248c9
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.
This is fine, though I think we should remove the svg icons, and update any other uses to use the png just to keep things simple/consistent.
I think it's only the info-outline icon that's reused, but you'll need to check.
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, removed. I checked and there are no more references to .svgs
so I think we're good. I'm also a fan of SVGs over PNGs in general, this is just a weird one.
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.
Just one small change request in comments - removing the old svg icons
import InfoIcon from './assets/info-outline-24-px.svg'; | ||
import CollapseIconOpen from './assets/expand-more-24-px.png'; | ||
import CollapseIconClose from './assets/chevron-right-24-px.png'; | ||
import InfoIcon from './assets/info-outline-24-px.png'; |
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.
This is fine, though I think we should remove the svg icons, and update any other uses to use the png just to keep things simple/consistent.
I think it's only the info-outline icon that's reused, but you'll need to check.
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 🚢
WEB-3418
Associated with tidepool-org/blip#1540