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

compose: Update compose placeholder text if recipients are changed. #15937

Merged
merged 3 commits into from Sep 4, 2020

Conversation

vinitS101
Copy link
Member

@vinitS101 vinitS101 commented Jul 27, 2020

The placeholder text is now updated every time recipients of Private Messages are changed (added or removed).

Fixes #15897.

GIFs or Screenshots:

For PMs:

compose_placeholder_group_pms

EDIT:

For stream messages:

compose_placeholder_streams_fixed

@zulipbot
Copy link
Member

Hello @zulip/server-compose members, this pull request was labeled with the "area: compose" label, so you may want to check it out!

@Gittenburg
Copy link
Collaborator

Do you also update the placeholder when changing which stream / topic you reply to? Because that missing is also part of #15897 and your commits message 64b380b only speaks of PMs.

@vinitS101
Copy link
Member Author

vinitS101 commented Jul 27, 2020

@Gittenburg Those already get updated when you move to the compose text-area.

compose_placeholder_streams

Started a discussion on czo - https://chat.zulip.org/#narrow/stream/6-frontend/topic/compose.20placeholder.20text/near/957729

@Gittenburg
Copy link
Collaborator

The PM placeholder also already updates when you focus the textarea. The problem I raise in my issue is that incorrect placeholders are confusing. In my opinion we could also just fix the placeholder to always be "Message" no matter whom / where you send it. That would still be better than having inconsistencies in the UI.

@ryanreh99
Copy link
Collaborator

Hi, I don't think we should add dependencies to the user_pill and input_pill files.

As we have the onPillCreate and onPillRemove functions which can be attached to the compose_pm_pill object.

@timabbott
Copy link
Sponsor Member

@vinitS101 ping on updating this.

@vinitS101 vinitS101 force-pushed the compose-placeholder-text branch 2 times, most recently from 14efe58 to 00add33 Compare July 29, 2020 18:11
@vinitS101
Copy link
Member Author

There was a bug in input_pill.js where onPillCreate did not get updated when a user pill was added in the PM compose box using typeheads. The second the commit fixes that issue, thanks to @ryanreh99.

The final commit is pending some tests, but works as expected (same as the GIF in the first comment). I'll add these tests and also a commit that updates the placeholder text for stream messages when the stream name is update.

@timabbott

@vinitS101
Copy link
Member Author

Added a commit that updates the placeholder text for stream name changes. Also added a GIF of the same to the first comment.

@vinitS101 vinitS101 force-pushed the compose-placeholder-text branch 2 times, most recently from 6cd9f0d to 33d3136 Compare August 27, 2020 16:09
@vinitS101
Copy link
Member Author

@timabbott please review this PR.

@timabbott
Copy link
Sponsor Member

I merged the first commit. @ryanreh99 can you review the onPillCreate change?

@vinitS101 it might help for you to expand that commit's commit message to make clear (1) exactly what the bug was and (2) how you're confident that nothing was relying on the old behavior; it's not obvious to me on a quick look.

@ryanreh99
Copy link
Collaborator

ryanreh99 commented Sep 1, 2020

The commit lgtm.
Previous behaviour was that the onPillCreate function was called after the individual pill object was created.
Now, we call it after creating and adding it to the pill container.

The commit title should be renamed to input pills instead of user_pills.

…head.

Previously, onPillCreate function was called after the individual
pill object was created.
Now, we call it after creating and adding it to the pill container.
Currently, compose box placeholder text for PMs only gets updated
when the focus shifts to it.
With this change, the text is now also updated if recipients are
added or removed.

Fixes zulip#15897.
Compose box placeholder text for streams currently updates when focus
is shifted to the text area.

With this change, it will also get updated when the stream name is
changed (it already updates if topic names are changed).
@vinitS101
Copy link
Member Author

vinitS101 commented Sep 1, 2020

Rebased and updated the commit messages to better explain the current and changed behavior.

@timabbott timabbott merged commit 140d24c into zulip:master Sep 4, 2020
@timabbott
Copy link
Sponsor Member

Great, merged, thanks @vinitS101!

Comment on lines +12 to +13
onPillCreate: input_pill.onPillCreate,
onPillRemove: input_pill.onPillRemove,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

input_pill.onPillCreate and input_pill.onPillRemove don’t exist. (input_pill is the module, not an instance.) What happened here?

@timabbott
Copy link
Sponsor Member

@ryanreh99 @vinitS101 it'd be great if one of you can figure out what the bug was with those onPillCreate methods; they were deleted in 03c409d, but it's possible another fix would be better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compose box does not update placeholder when changing recipients
6 participants