-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[PROPOSAL] New editing mechanism for small screens #76741
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
base: main
Are you sure you want to change the base?
Conversation
Code Review SummaryI've completed a comprehensive review of this PR. The implementation adapts message editing behavior based on screen size, using the bottom composer for narrow layouts and maintaining inline editing for wide layouts. Key Findings✅ Strengths:
Several inline comments have been posted with specific suggestions. The main areas of concern are:
Testing RecommendationsBeyond the test plan provided:
Overall, this is a solid implementation that addresses the UX issues with inline editing on mobile. The inline comments provide specific suggestions for improvement. |
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
Simulator.Screen.Recording.-.iPhone.17.-.2025-12-05.at.11.09.48.mov |
|
@Kureev I think you have linked wrong issue Can you please post videos of the behaviour before and after this change with clear explanation of what the changes are so the design and product team can discuss this? |
|
Oops, I lost track of this one so I’ll catch up on the context and review it later today. Currently, we still have a conflict on this PR. |
|
@Kureev Hi! I notice this PR references fix issue #63871 (16KB memory pages) but the changes seem to be about message editing UI (which matches issue #64085). Can you clarify if:
The proposal link points to PR 66240 which is about the same UI editing issue. |
|
@Kureev We have an issue where multiple message items cannot be edited, and after submitting all changes, the editing highlight still appears on the edited message. BeforeCleanShot.2025-12-12.at.01.14.14.mp4AfterCleanShot.2025-12-12.at.01.12.01.mp4 |
|
@Kureev There is still a conflict on this PR, along with feedback on the remaining issue. |
|
Hi all, sorry, was on a holidays, will look into it today |
|
I fixed the merge conflict with the |
|
We’re still waiting for @Kureev to handle this issue. |
|
@suneox i'm going to take this over from Alexey. I'll hopefully be able to address this later today, otherwise on Monday! |
Explanation of Change
Adapt message editing behavior based on screen size to match Slack-style UX. On narrow layouts, editing now uses the main bottom composer instead of inline editing inside the message list, preventing layout shifts, disappearing headers, and scroll jumps. On wide layouts, the existing multi-inline editing behavior is preserved.
Fixed Issues
$ #64085
PROPOSAL: #66240 (comment)
Tests
Offline tests
QA Steps
Same as Tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.npm run compress-svg)Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop