msglist: Use directional positioning for unread marker in RTL#1991
msglist: Use directional positioning for unread marker in RTL#1991gnprice merged 1 commit intozulip:mainfrom
Conversation
chrisbobbe
left a comment
There was a problem hiding this comment.
Thanks! Small comments below.
Also:
- Please say
Fixes #1245.in the commit message. For examples, use Greg's "secret" to usinggit log -p. - Please also post screenshots of what it looks like before this change, so we can see clearly the improvement being made. 🙂
lib/widgets/message_list.dart
Outdated
| } | ||
| } | ||
|
|
||
|
|
lib/widgets/message_list.dart
Outdated
| )), | ||
| ), | ||
| ), | ||
| ), | ||
| ), | ||
| ], | ||
| ); |
There was a problem hiding this comment.
This code-style change is unrelated and unhelpful; please remove it.
b28320e to
8cd3ad5
Compare
|
Thank you for the feedback. |
|
Bump on this part:
|
8cd3ad5 to
ff6061b
Compare
|
Thank You, I've updated the commit and pushed the revision. |
|
Please write it according to the project style; the |
ff6061b to
df5ae40
Compare
|
Thanks for the clarification, updated. |
|
Thanks! Marked for Greg's review. |
gnprice
left a comment
There was a problem hiding this comment.
Thanks @yash-agarwa-l for taking care of this, and thanks @chrisbobbe for the previous reviews!
Those before/after screenshots in particular are very helpful, thanks. It's especially helpful that they're very clean — the before and after differ only in the one way that's relevant.
This looks good; merging. One commit-message nit which I'll fix up:
- msglist: Use directional positioning for unread marker in RTL.
+ msglist: Use directional positioning for unread marker in RTL
In this repo we don't use a trailing period on the summary line of a commit message.
df5ae40 to
49d6226
Compare
|
I also just did a quick audit with I found 4 of them:
child: UnconstrainedBox(
alignment: Alignment.centerLeft,
child: Padding(
// TODO clean up this padding by imitating web less precisely;
// in particular, avoid adding loose whitespace at end of message.
padding: const EdgeInsets.only(right: 5, bottom: 5),
@yash-agarwa-l if you're looking for more issues to work on for contributing to Zulip, I think those would be a good list to pick up while this area is fresh in your mind from doing this issue. I'd suggest going in the order listed above — the quotes in particular I would guess look pretty wrong right now. And then a key element in a PR for any of these will be to post good before/after screenshots that clearly show the change, just like you did on this PR. |
|
Thanks so much for the review, the merge, and especially for auditing the code for these related RTL issues! I'm really happy contributing to Zulip. I'll start working through that list, beginning with the block quotations (Quotation widget), and will keep all your feedback in mind for the new PR. |
|
Looking at the screenshots in #2005, there's one other discrepancy I notice (in main, not new in that PR):
That isn't one of the places I found in my comment above. It looks like that's due to one more pattern to search for: |
|
Thanks! I ran git grep LTRB lib/ and found the one instance where the left and right values differed. I've opened PR #2038 to fix it. |
Switch to EdgeInsetsDirectional from EdgeInserts for recipent header date, it fixes the alignment in RTL layouts. Fixes the issue described here: zulip#1991 (comment)
|
@yash-agarwa-l Thanks again for all these fixes! Rerunning the search from #1991 (comment) now that the items mentioned there are all fixed, I find one more case which I missed earlier:
Would you be up for taking care of that? With good clear screenshots and a test, like you've been doing for these other issues. |
Previously, the `ScrollToBottomButton` was stuck on the right even in RTL layouts. This fixes it to work correctly in both RTL and LTR. Fixes the issue described here: zulip#1991 (comment)
|
Sure, happy to take care of that! I've made a PR for this fix (#2063) with the screenshots attached. |
|
OK, now that all those are fixed (thanks again @yash-agarwa-l!), I've gone and done a more comprehensive search: There are just two now that I think are suspect:
There are also a few classes of matches which look suspicious on their face, but I think are fine:
@yash-agarwa-l would you be up for resolving those last two cases? If you don't have a Mac for development then I think you won't be able to effectively test the second one — it relates to the idiosyncratic properties of Apple's proprietary emoji font. In that case we'll just leave that one for someone else to pick up. |
|
Thanks! I'll take care of both of those. I do have a Mac, so I can properly test the UnicodeEmojiWidget changes as well. |
On iOS and macOS, use AlignmentDirectional.centerStart instead of Alignment.centerLeft for the Stack containing Unicode emoji. Fixes the issue described here: zulip#1991 (comment)
On iOS and macOS, use AlignmentDirectional.centerStart instead of Alignment.centerLeft for the Stack containing Unicode emoji. Fixes the issue described here: zulip#1991 (comment)
On iOS and macOS, use AlignmentDirectional.centerStart instead of Alignment.centerLeft for the Stack containing Unicode emoji. Fixes the issue described here: zulip#1991 (comment)
Use AlignmentDirectional.bottomStart instead of Alignment.bottomLeft for the autocomplete dropdown positioning. This ensures the dropdown alignment respects text direction in RTL layouts. Fixes the issue described here: zulip#1991 (comment)
Use AlignmentDirectional.bottomStart instead of Alignment.bottomLeft for the autocomplete dropdown positioning. This ensures the dropdown alignment respects text direction in RTL layouts. This doesn't currently cause any change in user-visible behavior, because the autocomplete popup has the same width as the text input and so both the left and right edges are already aligned. Fixes the issue described here: zulip#1991 (comment)
Fixes zulip#1991. Use the email_auth_enabled server setting to conditionally render the username/password fields and the alternative auth divider.
Fixes #1245
This PR corrects the unread-marker alignment in RTL layouts.
Problem:
_UnreadMarker used LTR-specific properties (left: 0, Border(left: …)), causing the unread marker to always appear on the left, even for RTL languages like Arabic.
Fix
Therefore Replaced LTR-specific properties with directional equivalents:
PositionedDirectional(start: 0)
BorderDirectional(start: BorderSide(...))
This ensures the unread marker appears on the correct “start” side based on text direction.
Manually tested by setting:
locale: Locale('ar')
Verified correct behavior in both LTR and RTL layouts (screenshots included).
Before
LTR
RTL
After
LTR
RTL