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: Redesign limit indicator to show remaining characters count. #29830

Merged
merged 2 commits into from Apr 30, 2024

Conversation

N-Shar-ma
Copy link
Collaborator

@N-Shar-ma N-Shar-ma commented Apr 24, 2024

compose: Remove the message_too_long banner.

Since the banner only repeated what the disabled Send button's and the limit indicator's tooltips already said, it was redundant and has been removed.

compose: Redesign limit indicator to show remaining characters count.

Additionally, the text colors have been updated for both light and dark themes, it starts showing when 900 or less characters are left, as 999 was too soon, and has a tooltip to show the maximum characters limit.

Fixes: #28706.

Screenshots and screen captures:

Light theme:
Close to limit:
image
Over limit:
image

Dark theme:
Close to limit:
image
Over limit:
image

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.

@zulipbot zulipbot added size: L area: compose (misc) redesign Part of the webapp redesign project, including blockers. labels Apr 24, 2024
@zulipbot
Copy link
Member

Hello @zulip/design members, this pull request was labeled with the "redesign" label, so you may want to check it out!

@timabbott
Copy link
Sponsor Member

This lgtm implementation-wise other than the detail there; the one thing that maybe could be optimized further whether there's some tweak to positioning that would make it more obvious what this is. I think it's probably fine but @terpimost @alya FYI in case you want to suggest any changes before we merge this.

@alya
Copy link
Contributor

alya commented Apr 24, 2024

Looks good to me in manual testing!

@alya
Copy link
Contributor

alya commented Apr 24, 2024

By the way, this reminded me of #25271. Not a high priority, but I dunno if it's easy to do while this code is fresh in mind. We don't have an equivalent placement, but maybe to the right of the "Cancel" button would work?

We can leave it be if it's not super easy to do, though.

web/styles/compose.css Outdated Show resolved Hide resolved
@timabbott
Copy link
Sponsor Member

Please add the integration review label once you've fixed up these nits; I don't think we need to test-deploy this one.

@N-Shar-ma
Copy link
Collaborator Author

@zulipbot add "integration review"

@zulipbot zulipbot added the integration review Added by maintainers when a PR may be ready for integration. label Apr 28, 2024
Additionally, the text colors have been updated for both light and dark
themes, it starts showing when 900 or less characters are left, as 999
was too soon, and has a tooltip to show the maximum characters limit.

Fixes: zulip#28706.
Since the banner only repeated what the disabled Send button's and the
limit indicator's tooltips already said, it was redundant and has been
removed.
@timabbott
Copy link
Sponsor Member

I'm not sure why 900 would be the better number to use than 999, but going to go ahead and merge the design change and I'll post a query about that detail in #design, since it's a tiny nit to change.

@timabbott timabbott merged commit b36cdfd into zulip:main Apr 30, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: compose (misc) integration review Added by maintainers when a PR may be ready for integration. redesign Part of the webapp redesign project, including blockers. size: L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove denominator in character counter
5 participants