Skip to content
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

New compose box design #561

Merged
merged 1 commit into from Jun 9, 2017

Conversation

Projects
None yet
6 participants
@kunall17
Copy link
Contributor

kunall17 commented May 8, 2017

This is currently just for design suggestions and not for code review, after the correct design is finalized, I'll complete this!

First default screen

Stream

Private

@smarx

This comment has been minimized.

Copy link

smarx commented May 8, 2017

Automated message from Dropbox CLA bot

@kunall17, it looks like you've already signed the Dropbox CLA. Thanks!

@borisyankov

This comment has been minimized.

Copy link
Contributor

borisyankov commented May 8, 2017

I think that part of the UI is one of the weak points of the web app, and is made much worse by the nature of mobile - I really didn't like the usability in the Android app.

Typing is much harder, and way more error prone on mobile.
We should not require a person to know another person's email so they can write to them. I am not even sure why we would want to support the 'write to another person, while looking at different narrow' workflow.

Maybe we should just defer that to an in-person discussion with the other guys. It is such a weird concept, that if needed can be implemented better.

@borisyankov

This comment has been minimized.

Copy link
Contributor

borisyankov commented May 8, 2017

Also, we will have another line under the ComposeText (that is hidden right now, will be shown soon), and we have this always pinned subhead in the messages, and when the keyboard is popped, if we add another line we have almost no space for the messages.

What we definitely are missing is creating a new topic, but this concept needs work.

@kunall17

This comment has been minimized.

Copy link
Contributor Author

kunall17 commented May 8, 2017

We should not require a person to know another person's email so they can write to them.

Agreed, I'll make sure in the final version its based on the Names and not the emails

'write to another person, while looking at different narrow'

It often happens with me that I'm reading a stream and I need to message some other stream/person, it comes handy at that time.
So I kinda like this UI.
To make things easier whenever a narrow is made we can make it default that it's gonna send to that narrow!

Moreover this UI does not flashes all the time that you'll be sending messages to this group/private which looked kinda awkward in native android, hence looks better.
Also on tapping a message we can fill in that message recipient details, which is convenient to use!

@timabbott

This comment has been minimized.

Copy link
Member

timabbott commented May 8, 2017

It often happens with me that I'm reading a stream and I need to message some other stream/person, it comes handy at that time.

Yeah, this is something that we hard a lot from users; it's nice to be able to refer to some messages in a stream when PMing someone (e.g. to non-publicly point their attention at a public conversation).

I think maybe what we should do is have the default compose state be a super light experience that just sends to the current narrow, but have a button that expands compose to a full version, where you can specify where you want to send to as a new messages, whether private or stream with a new or existing topic, etc.

@borisyankov

This comment has been minimized.

Copy link
Contributor

borisyankov commented May 8, 2017

That makes sense; compared to the web version you are likely looking at just a few lines.

Creating a new topic could be a custom case - it can be one of the buttons next to 'image', 'photo', 'video' and when clicked it might replace them with input temporarily.

@kunall17

This comment has been minimized.

Copy link
Contributor Author

kunall17 commented May 9, 2017

@borisyankov So I should move the bubble (mode switcher from the 1st image) button to the ComposeOptions row and let the ComposeText be un touched?

@borisyankov

This comment has been minimized.

Copy link
Contributor

borisyankov commented May 10, 2017

@kunall17 so, we are splitting this task into two. First one, we start immediately. Next one will be 'soon but not next'.

Immediately:

  • if the user is typing in a stream narrow, show a button to change the topic

Not that soon:

  • fully custom 'compose screen' where you can pick stream, topic, recipients etc.
@kunall17

This comment has been minimized.

Copy link
Contributor Author

kunall17 commented May 11, 2017

Will start work from now, for the design modifications I pushed the mode changer button above

With options

On option change

Without divider:

Also what to go for, with or without long divider (Message box and compose options divider)?

@kunall17 kunall17 force-pushed the kunall17:patch-5 branch 2 times, most recently from f57dd56 to ab32f11 May 12, 2017

@kunall17

This comment has been minimized.

Copy link
Contributor Author

kunall17 commented May 12, 2017

@borisyankov
Updated, one thing is left which is on narrow changed the inputs should change or when mode is changed it should pick up the current narrow, i'll do it soon!

Screencast:
ezgif-3-81c231c925

@kunall17 kunall17 force-pushed the kunall17:patch-5 branch from ab32f11 to 68b01a4 May 14, 2017

@kunall17

This comment has been minimized.

Copy link
Contributor Author

kunall17 commented May 14, 2017

@borisyankov Updated the branch, lets keep these feature in this PR and other work in another PR

Added night theme support as well
screen shot 2017-05-14 at 5 28 22 pm

@kunall17 kunall17 changed the title [WIP] New compose box design New compose box design May 14, 2017

@kunall17 kunall17 force-pushed the kunall17:patch-5 branch from 68b01a4 to 31ca91f May 14, 2017

@zulipbot

This comment has been minimized.

Copy link
Member

zulipbot commented May 19, 2017

Heads up @kunall17, we just merged some commits (latest: 8213182) that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

@kunall17 kunall17 force-pushed the kunall17:patch-5 branch from 31ca91f to 6dba9a5 May 22, 2017

@zulipbot

This comment has been minimized.

Copy link
Member

zulipbot commented May 23, 2017

Heads up @kunall17, we just merged some commits (latest: 2578b4b) that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

@kunall17 kunall17 force-pushed the kunall17:patch-5 branch from 7286178 to 3fa3413 May 24, 2017

@zulipbot

This comment has been minimized.

Copy link
Member

zulipbot commented May 25, 2017

Heads up @kunall17, we just merged some commits (latest: 8297fdc) that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

@kunall17 kunall17 force-pushed the kunall17:patch-5 branch from 3fa3413 to 3d36602 May 25, 2017

@zulipbot

This comment has been minimized.

Copy link
Member

zulipbot commented May 25, 2017

Heads up @kunall17, we just merged some commits (latest: d1bc5e9) that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

@kunall17 kunall17 force-pushed the kunall17:patch-5 branch 2 times, most recently from 9dadad4 to e36cde9 May 25, 2017

@kunall17

This comment has been minimized.

Copy link
Contributor Author

kunall17 commented May 25, 2017

Updated :)

@kunall17 kunall17 force-pushed the kunall17:patch-5 branch 2 times, most recently from 9d12d6f to 9109b51 May 25, 2017

@borisyankov

This comment has been minimized.

Copy link
Contributor

borisyankov commented May 29, 2017

High-level changes needed:
Remove the stream picker and leave the Topic edit only.
Comment out the extra compose buttons.

Will do a detailed code review when this is done.

@kunall17 kunall17 force-pushed the kunall17:patch-5 branch 2 times, most recently from fc4f97e to 3b48206 May 31, 2017

@kunall17

This comment has been minimized.

Copy link
Contributor Author

kunall17 commented May 31, 2017

Updated the PR, though it does not uses the lastTopic you pushed recently.
Up for a review! :)

@kunall17 kunall17 force-pushed the kunall17:patch-5 branch from 3b48206 to 45d48d3 Jun 2, 2017

@zulipbot zulipbot added reviewed and removed needs review labels Jun 2, 2017

@borisyankov
Copy link
Contributor

borisyankov left a comment

The way users will pick Streams or PM recipients will be completely different than current web app or native Android.
Do not include any mode changing or 'thinking ahead' code.

Also definitely hide the extra icons that were from before since I can not approve the PR with them visible.

When anyone looks at this PR, all the code should be about selecting a 'topic' and there should not be a hint even at what is coming up or what other PR did exist.

@saketkumar
Copy link
Member

saketkumar left a comment

This is how the compose box is appearing in Android 6.
screenshot_20170603-000430

@kunall17

This comment has been minimized.

Copy link
Contributor Author

kunall17 commented Jun 2, 2017

@saketkumar95

screen shot 2017-06-02 at 12 24 24 pm 1

It looked fine on this emulator, can you specify more?

@kunall17 kunall17 force-pushed the kunall17:patch-5 branch from 45d48d3 to 72f996b Jun 2, 2017

@zulipbot zulipbot added needs review and removed reviewed labels Jun 2, 2017

@saketkumar

This comment has been minimized.

Copy link
Member

saketkumar commented Jun 2, 2017

Just forgot, It's Android 7.1 and i got this on personal device.

@saketkumar

This comment has been minimized.

Copy link
Member

saketkumar commented Jun 5, 2017

@borisyankov @kunall17 We really need to get this merged soon. My work uploading images/files will be done after this is merged.

@zulipbot zulipbot added reviewed and removed needs review labels Jun 5, 2017

"Language": "Language"
"Language": "Language",
"Topic": "Topic",
"Type a message here": "Type a message here"

This comment has been minimized.

@saketkumar

saketkumar Jun 5, 2017

Member

@kunall17 I think these should be added to all the other translations too?

This comment has been minimized.

@borisyankov

borisyankov Jun 5, 2017

Contributor

Good that you noticed, @saketkumar95
I added them for him with my last commit, so now it is OK.

@borisyankov

This comment has been minimized.

Copy link
Contributor

borisyankov commented Jun 5, 2017

Yeah, @kunall17 can you update the PR today?

@kunall17 kunall17 force-pushed the kunall17:patch-5 branch from 72f996b to a308319 Jun 7, 2017

@zulipbot zulipbot added needs review and removed reviewed labels Jun 7, 2017

@kunall17 kunall17 force-pushed the kunall17:patch-5 branch from a308319 to b0ddf5b Jun 9, 2017

@borisyankov borisyankov merged commit 3603254 into zulip:master Jun 9, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 59.908%
Details

@zulipbot zulipbot removed the needs review label Jun 9, 2017

@borisyankov

This comment has been minimized.

Copy link
Contributor

borisyankov commented Jun 9, 2017

The historical PR, merged at last :D

@kunall17

This comment has been minimized.

Copy link
Contributor Author

kunall17 commented Jun 9, 2017

Haha, finally :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.