compose-box: Fix compose-box from covering last messages of stream. #17082
Conversation
|
Hello @zulip/server-compose members, this pull request was labeled with the "area: compose" label, so you may want to check it out! |
|
@garg3133 sorry for the slow reply -- this PR got missed during my paternity leave. Do you have a chance to rebase it? |
garg3133
force-pushed
the
issue-16038
branch
2 times, most recently
from
46cf5f7
to
1209f1a
Compare
|
@timabbott I've rebased the PR, but sorry I couldn't test it after the rebase as my Ubuntu is broken right now and so I cannot run Zulip development environment on it. And for windows, I do not have enough free space on my hard-drive to setup a Vargant development environment. But when I created this PR, I tested in every possible way I could think of, which includes testing normally by writing a long message; resizing the screen with long message, resizing screen with short message and then making it long, resizing screen with compose box closed and then opening it and typing a long message; zooming in/out on my chrome browser for all three cases mentioned above; and it was working as great in all the cases. |
|
@timabbott I've managed to set up the development environment on my Ubuntu again and tested out these changes properly and these work completely as expected. |
|
@garg3133 hello, I viewed your PR. It's awesome, but I think this is a complicated approach to solve this. I suppose you have gone through the issue #17660. You can view my commit caa27ae in the PR #17667 that actually solves the issue itself in a more easy way. I have explained the solution in CZO here https://chat.zulip.org/#narrow/stream/137-feedback/topic/More.20full-featured.20compose.20box/near/1134101 I think we can have a discussion over this. One problem that I also think this PR can have is, it can also conflict with my work in the #17667. However, I think we should discuss on CZO more in detail. |
|
Hey @Signior-X, I looked at the commit you made but I don't think your commit solves the problem this PR is trying to solve. By applying your solution, the compose box is still covering a few messages at the bottom of the stream (even more than it covers now). And on small screens, it gives a very-very small window to scroll and read the messages.
Yeah sure, I'd be happy to discuss it with you on CZO. |
|
@Signior-X Taking a look at your complete PR, I don't think this PR would cause much issues to your work and any conflicts due to this PR can be easily resolved in your PR. The only conflict I can think of that this PR would cause is the resize of |
While writing a long message in compose-box, the last few messages of the current stream gets covered by the compose-box and it gets pretty annoying sometimes trying to figure out a way to read the last message of the stream while writing. Right now, the only way to get past this is to resize `compose-textarea` by using the resize tool at the bottom-right corner of the `compose-textarea`. But, that small resize tool is not always readily visible to the user. The proposed solution in this commit is to reset the `max-height` property of `#compose-textarea` everytime `bottom_whitespace_height` is resized such that the total height of `#compose` is always less than or equal to the height of `bottom_whitespace_height`. Doing this, the compose-box never covers the last message of the current stream. The only problem with this is that if the compose-box is closed at the time of bottom-whitespace resize, we cannot find the `compose_non_textarea_height` and so, we cannot reset the max-height of `#compose-textarea`. To solve this, max-height of `compose-textarea` is also reset everytime a new compose-box is opened according to the value of `bottom_whitespace_height` at that time. Thus, if the compose-box is already open at the time of bottom-whitespace resize, the max-height of `#compose-textarea` will also get reset at the same time, whereas, if the compose-box is closed at the time of bottom-whitespace resize, the max-height of `#compose-textarea` won't get reset at that time, but it will surely get reset whenever the user will open the compose-box. Tested on my Ubuntu Development Environment on Chrome and Firefox browsers. Fixes: zulip#16038.
|
Merged, after copying some of the explanation from the commit message to the comments, so that this code will make sense to folks without reading the commit history. Thanks @garg3133! |
While writing a long message in compose-box, the last few messages of the current
stream gets covered by the compose-box and it gets pretty annoying sometimes trying
to figure out a way to read the last message of the stream while writing. Right now,
the only way to get past this is to resize
compose-textareaby using the resize toolat the bottom-right corner of the
compose-textarea. But, that small resize tool isnot always readily visible to the user.
The proposed solution in this commit is to reset the
max-heightproperty of#compose-textareaeverytimebottom_whitespace_heightis resized such that the totalheight of
#composeis always less than or equal to the height ofbottom_whitespace_height.Doing this, the compose-box never covers the last message of the current stream.
The only problem with this is that if the compose-box is closed at the time of bottom-whitespace
resize, we cannot find the
compose_non_textarea_heightand so, we cannot reset themax-height of
#compose-textarea. To solve this, max-height ofcompose-textareais alsoreset everytime a new compose-box is opened according to the value of
bottom_whitespace_heightat that time.
Thus, if the compose-box is already open at the time of bottom-whitespace resize, the max-height
of
#compose-textareawill also get reset at the same time, whereas, if the compose-box isclosed at the time of bottom-whitespace resize, the max-height of
#compose-textareawon't getreset at that time, but it will surely get reset whenever the user will open the compose-box.
Tested on my Ubuntu Development Environment on Chrome and Firefox browsers.
Fixes: #16038.
Also tested by zooming-in/out the chrome which resizes the
bottom_whitespace_heightand thus automatically resets max-height ofcompose-textarea.