-
Notifications
You must be signed in to change notification settings - Fork 984
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
Toast notification design fixes #18468
Conversation
Jenkins BuildsClick to see older builds (56)
|
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 work!
:positive (if (= theme :light) | ||
:i/correct | ||
:i/correct-dark) | ||
:negative (if (= theme :light) | ||
:i/incorrect | ||
:i/incorrect-dark) |
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.
@alwx - The best way to handle colors in icons is using SVG.
(ns quo.components.icons.svg |
This will help us reduce the clutter of images/icons needed for each theme.
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.
Honestly, in the current form it just creates clutter somewhere else — namely in the namespace called quo.components.icons.svg
. In addition, those icons are very hard to be replaced because instead of putting them in resources we're now writing Clojure code for them.
I would say I'm gonna skip it for now because it feels wrong to replace an imperfect solution with something that's also imperfect and ugly.
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.
Also the discussion is here: https://discord.com/channels/1103692771585433630/1103692773363810317/1195414255038648360
83% of end-end tests have passed
Failed tests (6)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Expected to fail tests (2)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (40)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityOneDeviceMerged:
Class TestDeepLinksOneDevice:
Class TestActivityMultipleDevicePR:
|
Hi @alwx ! Thanks for your PR! Help me to clarify the point with platforms here:
Is it a copypaste from the form or your PR really needs to be tested on every of them? |
@mariia-skrypnyk sorry, no, fixed the description |
Any news regarding testing this one? |
Hi @alwx ! Thanks for updating description. I am starting to test ( I was on holidays 2 days and had no ability to start earlier). |
Hi @alwx ! But I do have question regarding Android: should it look the same there too? @Francesca-G help us please. |
@mariia-skrypnyk that's probably incorrect but the problem is on Android it really might depend on a phone. I tested it on emulator + on Pixel 7a, the position was right but I will check again what can be done to make it look good on all the devices. |
Same behavior should be applied on Android :) |
Thanks @alwx ! |
@mariia-skrypnyk sorry for the delay. Could you please check it again? |
Hi @alwx ! These places on Android look great as for me. But I am not sure this one is ok: @Francesca-G please, make a review of this one and let us know! |
@mariia-skrypnyk what would you say? Does it only happen when you close the drawer after you already saw the toast? In this case I also think it's pretty normal because the toast was already placed in the right position. |
Hi @alwx ! As @Francesca-G mentioned we have 2 issues.
Yes, user can get this view when he quickly closes the drawer AND also when he taps "Confirm selection" the banner is visible for some time. I tried to compare contact request vs images selection and we see them placed on the different line at the bottom. |
@mariia-skrypnyk as far as I understand, it happens ONLY when you close the image selection, so the popup actually appears on image selection screen and then stays for a couple of seconds in this state once you leave the image selection, is that correct/ |
yes! |
Yes, and we can consider this as expected behavior 👍 About the screenshot posted by @mariia-skrypnyk we still have an unsolved issue: The toasts should always have the same top margin, they should be aligned no matter how many lines of text they have. |
@mariia-skrypnyk I don't think it's feasible to even call it an issue, to be honest, because it seems to be a rare case. In addition, @Francesca-G wrote this: |
I totally agree! |
@alwx will you be able to fix this 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.
@alwx it looks good like this, thank you for your patience 🙏
fixes #17540
Platforms
Areas that maybe impacted
Functional
status: ready