Fix for: Editing workspace name to 1to3 characters adding extra line in the optional message when inviting users in Spanish#17878
Conversation
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
melvin-bot
Bot
requested review from
fedirjh and
techievivek
and removed request for
a team
|
@techievivek @fedirjh One of you needs to 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] |
|
@fedirjh @techievivek There are a couple of lines that weren't in my proposal: Firstly, I added this line to stop this unsightly flashing of the scrollbar that happens when you breaklines on web: Also I added this line as Android adds a tiny bit of padding to the right of the textInput which makes lines break incorrectly when typing thin characters such as Without With |
fedirjh
left a comment
fedirjh
left a comment
There was a problem hiding this comment.
Looks good. Left a few comments
| (this.props.hasError || this.props.errorText) && styles.borderColorDanger, | ||
| ], (finalStyles, s) => ({...finalStyles, ...s}), {}); | ||
| const maxHeight = StyleSheet.flatten(this.props.containerStyles).maxHeight; | ||
| const autoGrowHeight = this.props.autoGrowHeight && this.props.multiline; |
There was a problem hiding this comment.
autoGrowHeight should be added to baseTextInputPropTypes. Also I think we should remove this line and use this.props.autoGrowHeight directly, and for the input's prop multiline, we can update it to :
multiline={this.props.multiline || this.props.autoGrowHeight}There was a problem hiding this comment.
So when autoGrowHeight is applied , multiline should automatically set to true
| // When autoGrowHeight is true calculate textinput width or when multiline | ||
| // is not supplied calculate textinput height, using onLayout. |
There was a problem hiding this comment.
These comments are not so clear. Could you please update it ?
Co-authored-by: Fedi Rajhi <fedirjh@gmail.com>
|
@fedirjh Thanks for the suggestions, all updated. |
| this.props.autoGrowHeight && this.state.textInputHeight <= maxHeight ? styles.overflowHidden : styles.overflowAuto, | ||
| ]} | ||
| multiline={this.props.multiline} | ||
| multiline={this.props.multiline || this.props.autoGrowHeight} |
There was a problem hiding this comment.
Could you please update all other instances of this.props.multiline to also include this.props.autoGrowHeight, since the same rules that apply to multiline should also apply to autoGrowHeight? Perhaps we could create a local variable for this:
isMultiline = this.props.multiline || this.props.autoGrowHeight| onLayout={event => (this.props.autoGrowHeight && this.setState({width: event.nativeEvent.layout.width})) | ||
| || (!this.props.multiline && this.setState({height: event.nativeEvent.layout.height}))} |
There was a problem hiding this comment.
This can be simplified to :
onLayout = event => {
if (!this.props.autoGrowHeight && this.props.multiline){
return;
}
this.setState(prevState => ({
width: this.props.autoGrowHeight ? event.nativeEvent.layout.width : prevState.width,
height: !this.props.multiline ? event.nativeEvent.layout.height : prevState.height
}));
}Co-authored-by: Fedi Rajhi <fedirjh@gmail.com>
|
@fedirjh Updated, thanks. |
|
@Ollyws we missed implementing the App/src/stories/TextInput.stories.js Line 61 in 17c1dd8 |
|
I get this error on IOS native
|
|
@fedirjh This last commit should fix the layout error. |
|
@fedirjh One thing I missed earlier is that the textInput ignores whitespace at the end of the line, but with the zero-width space appended this doesn't happen in the hidden text, causing this issue: whitespaceissue.mp4The last commit fixes this by only adding the zero-width space if the last character is |
fedirjh
left a comment
fedirjh
left a comment
There was a problem hiding this comment.
Looks good , left few comments
|
@fedirjh And updated as you suggested. |
|
@fedirjh And updated as per the latest suggestion. |
|
@Ollyws There are some inconsistencies:
NativeIOSScreen.Recording.2023-05-03.at.8.10.49.PM.movScreen.Recording.2023-05-03.at.8.11.35.PM.movAndroidScreen.Recording.2023-05-03.at.8.16.38.PM.movWeb/ Mobile webSafariScreen.Recording.2023-05-03.at.8.09.47.PM.movChromeScreen.Recording.2023-05-03.at.8.19.52.PM.mov |
@fedirjh The line break doesn't work on production for me either. |
| {this.state.value || this.props.placeholder} | ||
| {/* We are appending a zero width space (\u200B) here as the browser will remove a trailing newline that doesn't | ||
| have any characters after it. This allows linebreaks to work properly on web when the user presses enter. */} | ||
| {`${this.state.value}${this.props.autoGrowHeight && (this.state.value.slice(-1) === '\n') ? '\u200B' : ''}` || this.props.placeholder} |
There was a problem hiding this comment.
| {`${this.state.value}${this.props.autoGrowHeight && (this.state.value.slice(-1) === '\n') ? '\u200B' : ''}` || this.props.placeholder} | |
| {this.state.value || this.props.placeholder} |
The line break doesn't work on production for me either.
Since this is not working on production either , is there any need for these changes?
There was a problem hiding this comment.
@fedirjh Yeah that's for non-mobile web.
There was a problem hiding this comment.
Also for non-mobile web it doesn’t work either on prod
There was a problem hiding this comment.
Try pressing shift+enter
There was a problem hiding this comment.
Not working
Screen.Recording.2023-05-03.at.11.49.05.PM.mov
There was a problem hiding this comment.
@fedirjh The question is, was the removal of pressing enter to linebreak intentional or is it a bug?
There was a problem hiding this comment.
I think this is a bug , but it's not related to the changes within this PR , as this can be seen in production and in different places e.g. Close account message input , I think this should be handled separately, and there is no need for these changes.
There was a problem hiding this comment.
Ok I'll get rid of these then.
There was a problem hiding this comment.
Have we reported this to the channel? But agree that this is a separate issue, so we shouldn't HOLD the PR for it.
There was a problem hiding this comment.
Missed this comment. We have an open ticket for this bug #18419
@fedirjh I resolved this issue by adding a fixed height to the textInput. It means we no longer needed scrollPaddingTop in Also, as we are getting closer to the 9 day penalty, perhaps we could request to not count the days between the 26th and the 1st toward the timeline? As during this period we considered the PR to be finished. |
|
@Ollyws Thanks for update, The native issue is fixed now , I will update the checklist with new recordings.
I am not the one who handles this, maybe you can raise that in the linked issue. |
|
@fedirjh Reverted the zero-width space. |
fedirjh
left a comment
fedirjh
left a comment
There was a problem hiding this comment.
The changes looks good and tests well. However, an unrelated issue was discovered during testing where the line break does not work as intended on the web. @Ollyws attempted to address the it here, albeit without success. It is recommended that this case be treated as a distinct issue and handled separately.
Over to you @techievivek
|
✋ 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/techievivek in version: 1.3.11-0 🚀
|
|
🚀 Deployed to production by https://github.com/roryabraham in version: 1.3.12-0 🚀
|
|
|
||
| return { | ||
| ...styles.overflowHidden, | ||
| height: maxHeight, |
There was a problem hiding this comment.
padding and borderwidth were not considered in maxHeight. It caused #18918 issue regression which is solved by https://github.com/Expensify/App/pull/19582/files
| }, | ||
|
|
||
| autoGrowHeightInputContainer: (textInputHeight, maxHeight) => ({ | ||
| height: textInputHeight >= maxHeight ? maxHeight : textInputHeight, |
There was a problem hiding this comment.
👋 Coming from #22583
We were missing a check for the minimum height when setting the input height. It only checked if the calculated height is greater than the maxHeight, but didn't consider the case where the calculated height is less than the minHeight.
We resolved this by calculating the height using
lodashClamp(textInputHeight, minHeight, maxHeight)
8 participants
Details
Fixed Issues
$ #17202
PROPOSAL: #17202 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)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.Screenshots/Videos
Web
MacOS_Chrome.mp4
Mobile Web - Chrome
Android_Chrome.mp4
Mobile Web - Safari
iOS_Safari.mp4
Desktop
MacOS_Desktop.mp4
iOS
iOS_Native.mp4
Android
Android_Native.mp4