-
Notifications
You must be signed in to change notification settings - Fork 985
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: action/reaction drawer UI issue #19630
Conversation
Jenkins BuildsClick to see older builds (61)
|
7a0aac9
to
7e275d9
Compare
7e275d9
to
8eb3358
Compare
8eb3358
to
fadbd84
Compare
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
a13f47f
to
e0b759b
Compare
PR is ready for design review @Francesca-G |
e0b759b
to
3b49758
Compare
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.
Sorry for that. It's a bug already in dev branch and it's fixed now. @Francesca-G |
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.
Looks good, I only left a comment here about the message bottom spacing:
19fdc1f
to
a25fe11
Compare
@Francesca-G bottom spacing should be fixed now |
b6c5a8d
to
d598d1f
Compare
@yqrashawn yup yup I don't mind taking a look on my side 👍 One note about what I remember about aligning these icons is that we use the author component in a few areas:
And it was tricky to get all of them to align correctly because they each use a slightly different layout that wrap the author component. So it might be worth checking that alignment is correct in those other areas 🙏 |
@yqrashawn okay I took on my side, and I think the icon is almost aligned by maybe slightly mis-aligned.
Also here's some screenshots from the other related screens that I mentioned.
|
554f824
to
7cbddd1
Compare
Signed-off-by: yqrashawn <namy.19@gmail.com>
Signed-off-by: yqrashawn <namy.19@gmail.com>
remove :peview? rename :in-reaction-and-action-menu? to :in-reaction-or-action-menu? Signed-off-by: yqrashawn <namy.19@gmail.com>
7cbddd1
to
654d756
Compare
Signed-off-by: yqrashawn <namy.19@gmail.com>
654d756
to
3bcfdb5
Compare
Hi @yqrashawn @seanstrom ! Please, let me know when I can retest it again. |
Hi @seanstrom, thanks for the detailed review. I've updated the commit and not the component should be pixel perfect on both platform
|
Hi @mariia-skrypnyk, ready for retest |
@yqrashawn thanks. |
85% of end-end tests have passed
Failed tests (6)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestWalletMultipleDevice:
Expected to fail tests (2)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Passed tests (44)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestWalletOneDevice:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePR:
Class TestDeepLinksOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
|
Failed e2e are not related. |
fixes #19278
Summary
quo/drawer-action
:accessibility-label
support, old:container
accessibility label is removed:type
for title and icon color (e.g.:danger
)quo/menu-item
withquo/drawer-action
Review notes
quo/drawer-action
is a new component (2 weeks old), it's not used anywhere until this PRlots of "drawer-action"s are using
quo/menu-item
, which should be upgraded to the new component, details at #19278 (comment)Testing notes
quo/menu-item
toquo/drawer-action
)Platforms
Areas that maybe impacted
action drawer for chat messages
reaction drawer (add reaction to message, then long press message reaction)
Before and after screenshots comparison
status: ready