action_sheet: Use directional border radius for reaction buttons#2010
action_sheet: Use directional border radius for reaction buttons#2010gnprice merged 1 commit intozulip:mainfrom
Conversation
|
Thanks, this looks great! Marking for Greg's review. I guess the ">" icon on the "more" button should really be flipped into "<" in RTL. Let's leave that out of scope for this PR, though. It's an instance of #1907. |
|
Thanks for the review! |
Thank you! I've edited your comment so it links to that PR, #2015, to make it easy to find from here. |
gnprice
left a comment
There was a problem hiding this comment.
Thanks! This looks great, with just small nits in the code style. Please go ahead and fix these and then I'll be glad to merge.
lib/widgets/action_sheet.dart
Outdated
| splashFactory: NoSplash.splashFactory, | ||
| borderRadius: isFirst | ||
| ? const BorderRadius.only(topLeft: Radius.circular(7)) | ||
| ? const BorderRadiusDirectional.only(topStart: Radius.circular(7)).resolve(textDirection) |
There was a problem hiding this comment.
nit: Let's inline textDirection:
| ? const BorderRadiusDirectional.only(topStart: Radius.circular(7)).resolve(textDirection) | |
| ? const BorderRadiusDirectional.only(topStart: Radius.circular(7)) | |
| .resolve(Directionality.of(context)) |
That means the lookup only happens in the case where it's needed. It's also hardly any longer on this line.
lib/widgets/action_sheet.dart
Outdated
| onTap: _handleTapMore, | ||
| splashFactory: NoSplash.splashFactory, | ||
| borderRadius: const BorderRadius.only(topRight: Radius.circular(7)), | ||
| borderRadius: const BorderRadiusDirectional.only(topEnd: Radius.circular(7)).resolve(textDirection), |
There was a problem hiding this comment.
nit: this puts important information past column 80; add a line break:
| borderRadius: const BorderRadiusDirectional.only(topEnd: Radius.circular(7)).resolve(textDirection), | |
| borderRadius: const BorderRadiusDirectional.only( | |
| topEnd: Radius.circular(7)).resolve(textDirection), |
(Could also inline textDirection the same as in the other place; but here that doesn't matter behavior-wise because it's needed unconditionally anyway.)
ac95dea to
6eb9051
Compare
|
Great point, that avoids an unnecessary lookup. Thanks! Applied. |
gnprice
left a comment
There was a problem hiding this comment.
Thanks! Looks good; merging, after fixing one nit below.
lib/widgets/action_sheet.dart
Outdated
| ? const BorderRadiusDirectional.only(topStart: Radius.circular(7)) | ||
| .resolve(Directionality.of(context)) |
There was a problem hiding this comment.
nit: indentation
| ? const BorderRadiusDirectional.only(topStart: Radius.circular(7)) | |
| .resolve(Directionality.of(context)) | |
| ? const BorderRadiusDirectional.only(topStart: Radius.circular(7)) | |
| .resolve(Directionality.of(context)) |
Previously, the reaction buttons in the message action sheet used hardcoded physical border radii. In RTL locales, this caused the rounded corners to appear on the inner edges of the button row instead of the outer edges. We now use `BorderRadiusDirectional` and resolve it against the context's text direction to ensure the correct corners are rounded regardless of layout direction.
|
Also fixed what appears to be a missing word in the commit message: |
6eb9051 to
8f4f27f
Compare
This fixes the border radius of the reaction buttons in the message action sheet for RTL languages.
Previously, the "First" button (e.g., thumbs up) and the "More" button (...) had hardcoded rounded corners. In RTL mode, these corners remained on the physical side, which resulted in the inner corners being rounded instead of the outer ones.
We now use
BorderRadiusDirectionalresolved against the text direction to ensure the outer edges are always the ones rounded.This addresses the third RTL issue identified by @gnprice in this comment.
Before
(Thumbs Up)
(More ...)
After
(Thumbs Up)
(More ...)