-
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
Add a button to expand the composer to full screen when the input has 3+ lines #9031
Conversation
Currently, expanding and collapsing the composer only works after refreshing the page, so I'll focus on fixing that. Please let me know what you think of my solution so far. |
Expanding and collapsing the composer works now. Next I want to automatically collapse the composer when a message is sent. Then I need to fix a bug where the composer doesn't reduce size when deleting lines from 3 to 0. |
iOS and Web are working well right now. Next I'll add support for Android and then I'll check to make sure that the other uses of the Composer (editing, emojis) still function correctly. |
Re-rolling PullerBear since I'm eager to get this merged and Luke is OOO for today. |
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 @stitesExpensify in version: 1.1.78-0 🚀
|
🚀 Deployed to production by @sketchydroide in version: 1.1.78-8 🚀
|
@@ -117,6 +127,9 @@ class ReportScreen extends React.Component { | |||
|
|||
componentWillUnmount() { | |||
clearTimeout(this.loadingTimerId); | |||
if (window.visualViewport) { | |||
window.visualViewport.removeEventListener('resize', this.viewportOffsetTop); |
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.
@neil-marcellini nice work on this PR and feature. I came across this use of window
in a cross-platform component and wanted to call out this section of the README.
We really shouldn't be seeing window
unless we are in an index.js
file and there's an index.native.js
that is doing a no-op.
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.
Ok, got it. Is it worth making an index.native.js
version of this component to fix this? Or could I make a withViewportOffsetTop
HOC that would have a web and native version?
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.
Both could work. Though, this component is pretty important and having two might get confusing. I would create a small lib that returns something like VisualViewport
we can call addEventListener()
on and for mobile it would just return an empty function.
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.
Could also return a cleanup method something like
index.js
function addResizeListener(callback) {
if (!window.visualViewport) {
return () => {};
}
window.visualViewport.addEventListener('resize', callback);
return () => window.visualViewport.removeEventListener('resize', callback);
}
index.native.js
function addResizeListener(callback) {
return () => {};
}
I feel like I've used this pattern before elsewhere to get around the cross-platform guidance.
👋 Heads up! As per the BugZero checklist, commenting to surface that this introduced a regression on Android outlined here. |
Thanks for the heads up. From the videos in the PR I'm pretty sure everything was aligned when this was merged, but I guess my changes were fragile. |
Details
When a user has typed 3+ lines into the composer (chat text input), show an expand button that will expand the composer to be full screen. The user can also collapse the composer if they like. The composer will automatically collapse when the message is sent. If the composer is full screen and the user deletes lines until there are less than 3, the composer will remain expanded until they press the collapse button.
Fixed Issues
$ #8423
Tests / QA Steps
All platforms:
Sign into NewDot with any account
Native Mobile only:
PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
filesSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
displayName
propertythis
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)PR Reviewer Checklist
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
).src/languages/*
filesSTYLE.md
) were followed/** comment above it */
displayName
propertythis
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)Screenshots
Web
web.mp4
Mobile Web
Android / Chrome
Android-Chrome.mp4
**iOS / Safari **
iOS-Safari.mp4
Desktop
desktop.mp4
iOS
ios.mp4
Android
android.mp4