Skip to content

Conversation

adnan-td
Copy link
Collaborator

@adnan-td adnan-td commented Jun 3, 2024

Removes the fixed height of the input field in compose DM recipient box. This commit makes the behaviour of recipient compose box equivalent to input pills used else where.

Fixes: #27688.

Screenshots and screen captures:

Before:

image

After:

Screenshot from 2024-06-03 19-46-36

Self-review checklist
  • Self-reviewed the changes for clarity and maintainability
    (variable names, code reuse, readability, etc.).

Communicate decisions, questions, and potential concerns.

  • Explains differences from previous plans (e.g., issue description).
  • Highlights technical choices and bugs encountered.
  • Calls out remaining decisions and concerns.
  • Automated tests verify logic where appropriate.

Individual commits are ready for review (see commit discipline).

  • Each commit is a coherent idea.
  • Commit message(s) explain reasoning and motivation for changes.

Completed manual review and testing of the following:

  • Visual appearance of the changes.
  • Responsiveness and internationalization.
  • Strings and tooltips.
  • End-to-end functionality of buttons, interactions and flows.
  • Corner cases, error conditions, and easily imagined bugs.

Removes the fixed height of the input field in compose
DM recipient box. This commit makes the behaviour of recipient
compose box equal to input pills used else where.

Fixes: zulip#27688.
@adnan-td
Copy link
Collaborator Author

adnan-td commented Jun 3, 2024

If still necessary, we can also implement character limit for the input field which would apply generally for all input_pills. Best way to do this would be to add a field on realm for storing the character limit for this use case and then cut off pasted text as already done in #29210.

@alya
Copy link
Contributor

alya commented Jun 3, 2024

I guess in theory the character limit should match the character limit for the pill. So we'd potentially have a different character limit for users vs. streams, etc. It seems OK to take the max of all the values if it's easier not to customize it for different pill types.

@alya
Copy link
Contributor

alya commented Jun 3, 2024

The screenshots look like an improvement to me!

@alya alya added the buddy review GSoC buddy review needed. label Jun 3, 2024
@adnan-td
Copy link
Collaborator Author

adnan-td commented Jun 4, 2024

It seems OK to take the max of all the values if it's easier not to customize it for different pill types.

One more thing I noticed is that pasting , separated long strings should still be allowed.
Tried implementing the character limit but not sure what to do in the above case.

@sujalshah-bit
Copy link
Collaborator

I manually tested it and everything looks good to me. One thing I noticed is that when you paste the text, the pasted text shakes a little bit. Although jitter is not related to this PR.

@adnan-td
Copy link
Collaborator Author

adnan-td commented Jun 5, 2024

One thing I noticed is that when you paste the text, the pasted text shakes a little bit.

This happens due to pasting tries to pillify the pasted text. If the pasted text is valid, it forms pills without shaking.

@alya
Copy link
Contributor

alya commented Jun 5, 2024

One more thing I noticed is that pasting , separated long strings should still be allowed.

Good point! Might not be worth worrying about a character limit, then.

@alya
Copy link
Contributor

alya commented Jun 5, 2024

@adnan-td Can you please ping your mentor for review?

@alya alya added mentor review GSoC mentor review needed. and removed buddy review GSoC buddy review needed. labels Jun 5, 2024
@adnan-td
Copy link
Collaborator Author

adnan-td commented Jun 5, 2024

@Ujjawal3 @shubham-padia can you please review this.

@Ujjawal3
Copy link
Collaborator

Ujjawal3 commented Jun 6, 2024

I have looked at the code changes and looks good to me.

@alya alya added maintainer review PR is ready for review by Zulip maintainers. and removed mentor review GSoC mentor review needed. labels Jun 7, 2024
@alya
Copy link
Contributor

alya commented Jun 7, 2024

Thanks! @karlstolley any feedback on this one, or is it ready for integration review?

@karlstolley
Copy link
Contributor

Great! We're ready for integration review on this; I just tested and this works well with 16/140, so that height: 20px value was going to need to go anyway. Thanks, @adnan-td!

@karlstolley karlstolley added integration review Added by maintainers when a PR may be ready for integration. and removed maintainer review PR is ready for review by Zulip maintainers. labels Jun 7, 2024
@timabbott timabbott merged commit ef65137 into zulip:main Jun 11, 2024
@timabbott
Copy link
Member

Merged, thanks @adnan-td, @Ujjawal3 and @karlstolley!

@adnan-td adnan-td deleted the issue/27688 branch June 30, 2024 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: compose (recipient pickers) Channel/DM recipient picker UI bug integration review Added by maintainers when a PR may be ready for integration. priority: high size: S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Long text spills out of DM recipients box
7 participants