reactions: (Actually) make text-emoji chip's height consistent with others #442
Conversation
|
There is a |
There was a problem hiding this comment.
Thanks for fixing this! Comments below.
and I think this version is more robust to different emoji sizes.
@sirpengi Can you elaborate on this? On their face, both the old and new code look to me equally robust to changing the emoji size — they both refer to the _squareEmojiSize constant.
lib/widgets/emoji_reaction.dart
Outdated
| // minHeight so text-emoji chips are at least as tall | ||
| // as square-emoji ones (probably a good thing). | ||
| minHeight: _squareEmojiScalerClamped(context).scale(_squareEmojiSize)), | ||
| child: Center(widthFactor: 1, |
There was a problem hiding this comment.
Hmm — it feels a little awkward to have this here, because it's basically saying the same thing as the CrossAxisAlignment.center on the Row.
lib/widgets/emoji_reaction.dart
Outdated
| @@ -159,12 +159,15 @@ class ReactionChip extends StatelessWidget { | |||
| mainAxisSize: MainAxisSize.min, | |||
| crossAxisAlignment: CrossAxisAlignment.center, | |||
| children: [ | |||
| // So text-emoji chips are at least as tall as square-emoji | |||
| // ones (probably a good thing). | |||
| SizedBox(height: _squareEmojiScalerClamped(context).scale(_squareEmojiSize)), | |||
| Flexible( // [Flexible] to let text emojis expand if they can | |||
| child: Padding(padding: const EdgeInsets.symmetric(horizontal: 3, vertical: 1), | |||
There was a problem hiding this comment.
After this change, there's 1px vertical padding on each child of the Row. Can that padding move outward?
It seems like it'd be equivalent to have instead 1px vertical padding around the Row; and then in fact it should commute with the LayoutBuilder too, and be able to merge into the outer Padding.
After that, we wouldn't need the "+ 2" adjustment, so I think we could drop the second commit and that redundant-looking Center.
chrisbobbe
force-pushed
the
pr-text-emoji-minimum-height
branch
from
ec4a21f to
d5405be
Compare
|
Thanks for the review! Revision pushed. |
| // Added vertical: 1 to give some space when the label is | ||
| // taller than the emoji (e.g. because it needs multiple lines) |
There was a problem hiding this comment.
As Greg suggested:
https://github.com/zulip/zulip-flutter/pull/442/commits/ec4a21fdccdb21774ca656082fa921161d6b1ae0#r1442492101
Link is broken — GitHub links like …/pull/NNN/commits/HHH/… are unstable, because GitHub breaks them if the commit is no longer in the latest revision of the PR. Instead link to discussions on the "Conversation" tab of the PR, like #442 (comment) .
There was a problem hiding this comment.
Ah indeed. I knew that, but missed it in this case; thanks for the catch. 🙂
As Greg suggested: zulip#442 (comment) We drop a comment about giving 1px vertical padding on the label when it's taller than the emoji (e.g., because the label needs multiple lines). We still give that padding, and that's a difference from the web app -- and so arguably worth noting in the code -- but it's a small point, and clearly an improvement, so it's probably OK to drop the comment and let the reader use their attention on more important things.
chrisbobbe
force-pushed
the
pr-text-emoji-minimum-height
branch
from
d5405be to
5421518
Compare
If we don't like the refactor in the second commit, we can drop it and #441 will still be fixed. 🙂
Fixes: #441