-
Notifications
You must be signed in to change notification settings - Fork 3k
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 inconsistent margins for reacted emojis to different sort of comments #22010
Fix inconsistent margins for reacted emojis to different sort of comments #22010
Conversation
…/18681-inconsistent-margins-2
…/18681-inconsistent-margins-2
It is still a draft PR. Once it is ready, I will open it for review 🙂 |
@abdulrahuman5196 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
@abdulrahuman5196 Just a friendly reminder that the PR is ready for review 🙂 |
…/18681-inconsistent-margins-2
Sorry for the delay @rezkiy37 Will close the review before tomorrow morning. |
Reviewer Checklist
Screenshots/VideosWebMobile Web - ChromeMobile Web - SafariDesktopiOSAndroid |
NAB: @rezkiy37 Kindly mark Offline tests and QA tests as "Same as tests". Offline tests also should be present for this change since its expected to work offline |
Done. |
@abdulrahuman5196 Just a friendly reminder 🙂 |
Working on this now |
@rezkiy37 When we hover the message, the hover doesn't have any space top or bottom as its like attached to the message which is odd. Screen.Recording.2023-07-13.at.3.03.07.PM.mov |
Yes, it works this way, because the messages are separated by a separator component of FlatList, not via paddings. |
This implementation follows the proposal, that was approved. So, we should decide or ask someone who care about UX. |
@rezkiy37 I think hover should have padding as per the current working behaviour, it is odd without padding for hover. @conorpendergrast Could you kindly tag an UX person to confirm on the same? #22010 (comment) |
@abdulrahuman5196, I misunderstood. "hover should have padding" and "it odd with padding for hover". Does not it contradict? |
@rezkiy37 Sorry. It was a typo, updated the comment - #22010 (comment) I am aligned to have some amount of padding top and bottom on hover similar to current behaviour. |
Well, let me add them and tweak the separator. |
Thank you @rezkiy37 |
…/18681-inconsistent-margins-2
@abdulrahuman5196, just to clarify the next steps:
|
@rezkiy37 Why would this be? Is it not possible to tweek the separator and message hover size? Any blockers there? |
I was expected to fix the hover issue alone, otherwise the implementation here seems fine AFAIK. |
@rezkiy37 I am still seeing issue with whipser messages And moreover I am not sure on understanding what we are doing as a fix for whisper messages. Could you explain what are we trying to do specifically for whispers alone. |
…/18681-inconsistent-margins-2
@abdulrahuman5196, updates: My bad, it is one more case. I added a logic to understand what the previous action is whisper. It fixed a case when the current message is a whisper and the previous one as well, but they are not in one group. Example
whisper.mov
Yes, sure. The app highlights separators between whisper actions when they are in a group. It creates this solid effect. |
…/18681-inconsistent-margins-2
…/18681-inconsistent-margins-2
@rezkiy37 Still I am seeing issues with hover on whisper Current issues
Screen.Recording.2023-08-13.at.9.55.37.PM.mov@Beamanator We have been trying so much to fix the hover issue, but even after multiple iterations we are still unable to perfect the solution. Should we try for a different approach to solve the issue? Or any advice on path forward? |
@abdulrahuman5196, yeah, I see. I agree to wait a comment from @Beamanator to consider any next steps here. |
…/18681-inconsistent-margins-2
…/18681-inconsistent-margins-2
@Beamanator Waiting on your comments here? |
@Beamanator Reference #22010 (comment) |
…/18681-inconsistent-margins-2
Sorry for the delay gents, it's been quite a busy few weeks prepping for some conferences - I will try to get back to y'all today 🙏 |
Hmm I can see how these types of changes (to Report action components) can run the risk of regressions since there's lots of complicated logic all over the place - I think it definitely would make sense for @rezkiy37 to take a bit of time, step back and reevaluate if there's another solution. I don't currently have time to look into what exactly would be the best way forward for this, but that's why we hire others for that work :D One strategy I'm thinking could work well:
Thoughts? |
Makes sense. I believe we should do these steps. Also, I will ask some help of my Callstack teammates. |
@abdulrahuman5196, @Beamanator, I still think about: What alternative solutions did you explore?There is another solution how to fix these weird margins only.
Since, the main approach was not good, I believe in this one. Thoughts? |
@rezkiy37 Sorry I missed this one. I will check on the alternative solution and update sooner |
Great work guys! @abdulrahuman5196 I've followed the conversation up until this point and before I look into any further proposals into this, I would also like to see how @rezkiy37 most recent proposal plays out. Perhaps I could come in at this point to see how I can assist. would also be waiting on your thoughts... |
…nconsistent-margins-2
@rezkiy37 I tried this #22010 (comment) But for me it seems like, there isn't visible UI changes happening. I am sure if I am missing something. If possible could you kindly provide screenshots of what is previous and new change in UI? |
Let me please provide more info here. |
Initially, our goal here to align spacing between a message and reactions for different type of messages. Actually, there is a component with an inconsistency - BasePreRenderer. It happens, because it applies an additional bottom margin. So, I am no the latest
|
Thank you for the clear explanation @rezkiy37 here #22010 (comment) This was the alternate solution proposed during the proposal review. We have already tried other better solutions but unfortunately, due to other issues we weren't able to proceed on the same. IMO, I think at this state this alternate is a good enough fix given the main solution(better solution) has multiple side effects preventing its implementation. 🎀 👀 🎀 cc: @Beamanator |
Cool, thank you! |
Thanks @abdulrahuman5196 and @rezkiy37 for the continued effort here, looking forward to the next PR 👍 |
Details
Fixed Issues
$ #18681
PROPOSAL: #18681 (comment)
Tests
Offline tests
Same as tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android