-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
allow Request money 'Description' to accept multiline #21664
Conversation
@aimane-chnaif 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] |
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.
Please remove any changes related to money request header. It will be handled in #19389 and out of scope for this PR.
Here, we should fix only input part.
@youssef-lr can this be separate issue? Same happens on task description from #19377 which you reviewed. web.movLast line cursor overlaps with bottom border but fine when at least 1 letter |
removed. |
@ahmedGaber93 mSafari not working. can you investigate? I also tested task description input. There's another weird bug but at least multiline works. msafari-bug.mov |
This issue also happened with me in web.
and this subscription also work in inputs
My proposal for this is:Adding empty EnterKey subscription in componentDidMount() {
const enterConfig = CONST.KEYBOARD_SHORTCUTS.ENTER;
this.unsubscribeEnter = KeyboardShortcut.subscribe(
enterConfig.shortcutKey,
() => {},
enterConfig.descriptionKey,
enterConfig.modifiers
);
}
componentWillUnmount() {
if (this.unsubscribeEnter) {
this.unsubscribeEnter();
}
} |
This conflicts with #21661 |
This issue #21661 is related to tasks not money request, why it conflicts? @aimane-chnaif |
I meant that's the reason why inconsistent behavior between task description input and money request description input |
@aimane-chnaif I will check this #21664 (comment) |
@ahmedGaber93 let's follow #21661 (comment) |
|
Thanks for update. |
@aimane-chnaif I applied the same fixes here #21664 (comment) in this PR, you can review now. |
@ahmedGaber93 do you think all task description bugs are gone now except #21271? |
@aimane-chnaif yes we now have the same behavior in task description and request money description except "shift + enter" in web, it not return new line on request money description but do in task description in web. And here is my root cause and proposal for it #21664 (comment) |
Agree. This is hidden bug and we haven't noticed before enabling multiline. |
Fix pushed
Can't reproduce it on native android |
thanks |
src/libs/actions/IOU.js
Outdated
@@ -116,7 +116,7 @@ function buildOnyxDataForMoneyRequest( | |||
key: `${ONYXKEYS.COLLECTION.REPORT}${iouReport.reportID}`, | |||
value: { | |||
...iouReport, | |||
lastMessageText: iouAction.message[0].text, | |||
lastMessageText: ReportUtils.formatReportLastMessageText(iouAction.message[0].text), |
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.
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.
Actually I think this can be separate issue to fix inconsistency between optimistic data and backend data for last message text with multiline description.
Can't we just prevent wrap here?
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.
Can't we just prevent wrap here?
Are you mean remove new lines "\n" from message text?
If you mean that the lastMessageText will not be inconsistent with backend, because backend return only first line of message text.
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.
Can't we just prevent wrap here?
I meant to keep that optimistic value as is now but just fix display issue of wrapped
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.
the wrap happened here because numberOfLines={1}
does not work as expected when <Text/>
contains \n
and whiteSpace: 'pre'
just not break the long lines but not affected on \n
For that, we can fix it by replace styles.pre
with styles.noWrap
here
App/src/components/LHNOptionsList/OptionRowLHN.js
Lines 87 to 92 in 4362fe7
const alternateTextStyle = StyleUtils.combineStyles( | |
props.viewMode === CONST.OPTION_MODE.COMPACT | |
? [textStyle, styles.optionAlternateText, styles.pre, styles.textLabelSupporting, styles.optionAlternateTextCompact, styles.ml2] | |
: [textStyle, styles.optionAlternateText, styles.pre, styles.textLabelSupporting], | |
props.style, | |
); |
This will remove the new lines "\n" from message text and fix the wrap issue, but inconsistency with backend will still issue.
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.
Workaround cannot be a quick fix.
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.
I can't find a quick fix right now. What is the next step now?
- use
styles.noWrap
and fix wrap issue partially with inconsistent result on iOS. - do nothing and keep wrap issue separately?
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.
do nothing and keep wrap issue separately?
We can't do this because it looks weird and our PR might need to be reverted.
use styles.noWrap and fix wrap issue partially with inconsistent result on iOS.
This is still concerned but we would consider this not a deploy blocker I believe.
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.
Yes, I update the code to use styles.noWrap
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.
styles.noWrap
caused regression - #27586
Reviewer Checklist
Screenshots/VideosWebweb.movMobile Web - Chromemchrome.movMobile Web - Safarimsafari.movDesktopdesktop.moviOSios.movAndroidandroid.mov |
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.
Follow up issues:
- iOS alternativeText in LHN shows only first line, while all other platforms shows full text in one line (
...
at the end if overflows) (allow Request money 'Description' to accept multiline #21664 (comment)) - LHN alternativeText, report header title don't preserve spaces and tabs (caused by replacing
pre
withnowrap
) - inconsistent report.lastMessageText between optimistic data (full text) and backend (only first line)
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! Just one small grammatical change
); | ||
} | ||
|
||
unSubscribeToKeyboardShortcut() { |
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.
unSubscribeToKeyboardShortcut() { | |
unSubscribeFromKeyboardShortcut() { |
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.
Renamed
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!
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/stitesExpensify in version: 1.3.59-0 🚀
|
🚀 Deployed to staging by https://github.com/stitesExpensify in version: 1.3.60-0 🚀
|
🚀 Deployed to staging by https://github.com/stitesExpensify in version: 1.3.60-0 🚀
|
🚀 Deployed to production by https://github.com/luacmartins in version: 1.3.59-5 🚀
|
🚀 Deployed to production by https://github.com/luacmartins in version: 1.3.60-3 🚀
|
@@ -51,7 +51,7 @@ function DisplayNamesWithToolTip(props) { | |||
return ( | |||
// Tokenization of string only support prop numberOfLines on Web | |||
<Text | |||
style={[...props.textStyles, styles.pRelative]} | |||
style={[...props.textStyles, styles.pRelative, props.numberOfLines === 1 ? styles.noWrap : {}]} |
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.
styles.noWrap
is not needed here #32555 (comment)
Details
Fixed Issues
$ #19872, #25780
PROPOSAL: #19872 (comment)
Tests
Offline tests
N/A
QA Steps
Same as Tests step.
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
Screen.Recording.2023-08-23.at.11.32.22.AM.mov
Screen.Recording.2023-08-24.at.4.26.25.AM.mov
Mobile Web - Chrome
Screen.Recording.2023-08-23.at.12.34.57.PM.mov
Screen.Recording.2023-08-24.at.4.29.06.AM.mov
Mobile Web - Safari
Screen.Recording.2023-08-23.at.12.01.23.PM.mov
Screen.Recording.2023-08-24.at.4.38.05.AM.mov
Desktop
Screen.Recording.2023-08-23.at.11.39.48.AM.mov
Screen.Recording.2023-08-24.at.4.35.58.AM.mov
iOS
Screen.Recording.2023-08-23.at.11.59.30.AM.mov
Screen.Recording.2023-08-24.at.5.24.09.AM.mov
Android
Screen.Recording.2023-08-23.at.12.54.19.PM.mov
Screen.Recording.2023-08-24.at.4.30.45.AM.mov