emoji: Ensure consistent row height for plain text emoji theme.

[yash-agarwa-l]

Fixes #1587

Fix vertical layout shift in plain-text emoji picker by enforcing 44px minimum row height to match icon-based rows.

Before

Default

Plain Text

After

Default

Plain Text

[chrisbobbe]

CI is failing; I recommend running tools/check locally and fixing the issues it finds.

[yash-agarwa-l]

I ran tools/check locally and all checks/tests pass on my end.
Please let me know if there’s anything else I should adjust.

Thanks!

[yash-agarwa-l]

I've rebased on the latest main branch to pick up the dependency fixes.
CI is now passing.

[chrisbobbe]

Thanks! Just one small comment below, and I'll mark this for Greg's review.

[chrisbobbe]

Suggested change

	]),
	)));
	]))));

[gnprice]

Thanks @yash-agarwa-l for taking care of this, and @chrisbobbe for the previous review!

One comment below, and a pair of nits in the commit message:

emoji: Ensure consistent row height for plain text emoji theme.

Fixes #1587

Should instead be:

emoji: Ensure consistent row height for plain text emoji theme

Fixes #1587.

See examples of previous commits, using git log or a graphical Git client:
https://github.com/zulip/zulip-mobile/blob/main/docs/howto/git.md#git-log-secret

That's helpful for style examples to follow, but also much more broadly for exploring the codebase more deeply and understanding what changes we're making.

[gnprice]

It's best to have the code make clear what our reasoning is for choosing a given value, as far as that's possible. Where does the value 20 come from here?

[yash-agarwa-l]

Thank you! I have added the reasoning and refactored the commit message.

[gnprice]

Thanks for the revision! Comments below.

A nit in the commit message: the sentence "Fixes #1587." goes at the beginning.

[gnprice]

In my experience seeing codebases evolve over time, this sort of comment is unfortunately often worse than useless. The problem is that it says something about what another piece of code does — but that other piece of code might well change in the future, and there's nothing that would cause this comment to get updated to match. So the comment has a high risk of becoming wrong in the future and misleading people reading it.

Instead, given this reasoning for the value, a good way to express that reasoning is to put this expression right next to the place where the other values appear which it's meant to be related to. So in this case a good start would be to inline _rowMinHeight at the one place it's used, which is close to where the EdgeInsets.all(10) appears.

Then you could also add a comment explaining this same reasoning, but without mentioning the actual numbers 10 or 20. That way if the numbers get changed in the future, the comment doesn't become wrong; it's up to the reader to consult the actual code to see the authoritative answer for what the numbers are. So e.g.:

        child: ConstrainedBox(
          // Set the min height so that the height remains the same
          // when glyph is null vs. non-null.
          constraints: const BoxConstraints(minHeight: _emojiSize + 20),
          child: Row(spacing: 4, children: [
            if (glyph != null)
              Padding(
                padding: const EdgeInsets.all(10),
                child: glyph),

[gnprice]

Stepping back, though: I don't think this is actually the right reasoning here.

A given user will be seeing either the text or the non-text case — this is a user-level setting, not something that might vary from one row to the next. So there's no reason that the height of the rows needs to be the same between the two cases; after all, the design is already different between the two cases.

Instead, as the issue says, the problem is that the touch targets in the text-emoji case aren't tall enough. They need to be taller, so that people can tap on the one they want without hitting one of its neighbors instead.

This should therefore have a min-height equal to our normal minimum size for a touch target. To find that value, do a search like: git grep -iP 'touch.?target'.

[yash-agarwa-l]

Thank you so much, revision pushed.

[gnprice]

Thanks! This looks good; merging, with one comment fix:

         child: ConstrainedBox(
-          // Ensure the row meets the minimum touch target height,
-          // when glyph is null vs. non-null.
+          // Ensure the row meets the minimum touch target height.
           constraints: const BoxConstraints(minHeight: 44),

The rest of the sentence isn't really trying to draw a contrast between different cases related to glyph, so the second half was out of place.