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

info_density: Calculate values for vertical alignment and apply to emoji. #30050

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

karlstolley
Copy link
Contributor

@karlstolley karlstolley commented May 10, 2024

This PR calculates values necessary to properly fit and vertically align inline-block elements within any given line height and font size combination. Those values are then applied as an example to inline emoji, though the techniques and values here are strong candidates to be extended to other blocks appearing within the message area (e.g., todo widget checkboxes, <time> elements).

Additionally, previous fiddly top and margin values applied to emoji have been removed here, so that similarly fiddly adjustments are made unnecessary as users adjust information density values. The consequence of this is that emoji are now properly fitted to the line height, making them appear smaller than previously (though better in line with the size of surrounding text).

CZO discussion

To test this PR in dev, you'll see the default values just by running dev on this branch. The database-stored legacy font size and line-height equivalents can be made active by deselecting Dense mode under Settings > Preferences. To see the 16px/140% version, set those values in the Message-area font size (px) and Message-area line height (%). Same with any other reasonable combination of font size and line-height you might be curious to examine.

Screenshots and screen captures:

Before After (legacy font size/line height values)
legacy-emoji-before legacy-emoji-after-calculated-offset
16px/140% 100px/100% (exaggerated, line-fitted) 100px/300% (exaggerated, font-size-clamped)
emoji-16-140 emoji-100-100 emoji-100-300

Showing shift-free text at 16px/140%

16-140-shifts

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.

@karlstolley karlstolley added area: emoji Emoji in markup, emoji reactions, emoji picker, etc. chat.zulip.org review Added by maintainers when a PR requires chat.zulip.org testing to proceed. labels May 10, 2024
@zulipbot
Copy link
Member

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

@timabbott timabbott added the deployed on chat.zulip.org Added by maintainers when a PR is currently being tested on chat.zulip.org. label May 14, 2024
@timabbott
Copy link
Sponsor Member

Test-deploying on chat.zulip.org.

@karlstolley karlstolley force-pushed the em-fitted-inline-emoji branch 4 times, most recently from b76960b to 5e3b904 Compare May 16, 2024 16:52
@zulipbot zulipbot added size: XL and removed size: L labels May 16, 2024
@karlstolley
Copy link
Contributor Author

Okay. I have updated this with an entirely different approach, as discussed on CZO--which discussion I'll be updating shortly with some of the things I've discovered here and want to share more widely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: emoji Emoji in markup, emoji reactions, emoji picker, etc. chat.zulip.org review Added by maintainers when a PR requires chat.zulip.org testing to proceed. deployed on chat.zulip.org Added by maintainers when a PR is currently being tested on chat.zulip.org. size: XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants