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

Fix Repost by username ellipsis #5630

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

Conversation

MetaflameDragon
Copy link

This addresses #5624.

There was a missing flexbox around the "Reposted by" text, somehow that caused issues for ellipsis with the username. Unfortunately, a flexbox forcefully trims spaces, including the space after "Reposted by ". All of this also applies to "Reply to ".

I messed around with a few solutions and nothing else seemed to work properly (without possibly breaking something else). The ideal solution would be to give the parent white-space: pre instead of nowrap, but it seems to be controlled strictly by numberOfLines and React Native doesn't seem to like accepting it as a direct style. The resulting whitespace collapse behavior trims the space from "Reposted by " and its translations, and so far only nbsp worked.

A nbsp (from a constant) is already used in another place, but here it must be part of the translation string right now (actually, the whole "Reposted by " string), which... I'm not sure how to resolve correctly. Regardless, this fixes the behavior at least, maybe something else can be figured out with the translation strings.

I also suspect that this isn't an issue on mobile, as ProfileHoverCard seems to become passthrough (?), so the Text elements get inlined correctly. In this case, there are two divs in between, which is part of what causes this behavior. There's an inline props on the hover card that seems to go unused - maybe it could be related to this issue of inlining contained text?

This was made to be similar to the handle component structure. The Repost reason text was missing a flexbox, and adding it back seems to make ellipsis work again. Slightly breaks due to white-space-collapse though.
Forcing 1 line really doesn't want to keep the space, since it gets set to collapse along with enabling ellipsis. Changing it to a nbsp fixes this, and it can be non-breaking since it must be exactly 1 line anyway. This breaks current translations though.
@MetaflameDragon
Copy link
Author

Gonna try to poke this more tomorrow. I'll be honest, I'm not too experienced with the stuff used here, particularly how translation strings work, so I'll have to read up on that more first.

To clarify, the ellipsis thing is fixed here, but due to using a non-breaking space, translation strings are broken instead. I read about Trans tags providing custom components, which is necessary for using nbsp within translation strings since some of them use text on both sides of the username (and so the username component must be referenced in the strings).

This would all also be much more straightforward if the whole "Reposted by username" string were within the hover card tag instead of just the username (since the hover card breaking the string up is part of what causes the issue), but I'll see about getting the nbsp into the translation strings. Ditto for "Reply to username".

@estrattonbailey
Copy link
Member

Mind including an example? Screenshots or links to specific posts would be awesome!

@MetaflameDragon
Copy link
Author

See the issue (#5624) for screenshots. A link to an existing example would be the current reposts on my test account (yeah I made a test alt account just for this lol). Chrome & Firefox versions are also specified in the issue report. I don't believe I have any cosmetic filtering or other modifications enabled (uBlock Origin is disabled etc.), so that shouldn't be a factor.

Make sure to resize the browser window and/or zoom in if the username still doesn't get truncated. The new font on https://main.bsky.dev is significantly narrower, for example. This bug occurs both in "Reposted by" and "Reply to", both of which are in the linked example.

For a bit of technical detail, check the nearest flexbox around the username in the "Reposted by" text, you should see that neither the text element nor the flexbox shrink with the parent properly. This, for some reason paired with the fact that the Hover Card wrapper divs have flexboxes, causes the ellipsis to break. Removing the flex from the wrapper divs didn't seem like a clean solution though, so I tried making the child elements actually flex properly, which... eats leading/trailing spaces (unless they're a nbsp).

Screenshot with a flexbox outlined
image

Hope that answers the question!

If you're wondering why French - it's the only other language from this list that I can read lol
@MetaflameDragon
Copy link
Author

MetaflameDragon commented Oct 9, 2024

All locales have been updated to work with the new string & tested in my browser. Ellipsis now behaves as expected for usernames in "Reposted by" and "Reply to". Locales where the username was at the start or in the middle also truncate as expected, only the name gets ellipsized.

Tested on: https://main.bsky.dev/profile/metaflame.dev/post/3l5tvlhwqwo2p (my & my test alt's usernames are significantly longer right now)

This was tested in the webapp in Firefox and Chrome on both desktop (see #5624 for device details) and on mobile.

This was NOT tested in the native app and in other browsers. Given the behavior of the post hover card (which seems to become a no-op passthrough on mobile native) I think native should work fine.

Unrelated observed issue: Repost icon left of the text shrinks when window width decreases. This is previous behavior, really minor, and unrelated to this bug.

@MetaflameDragon MetaflameDragon marked this pull request as ready for review October 9, 2024 00:24
@surfdude29
Copy link
Contributor

All locales have been updated to work with the new string

FYI the Cantonese (zh-HK) localization was just recently merged in #5479 👀

@MetaflameDragon
Copy link
Author

Thanks for the heads-up! I didn't catch that in my conflict resolve (because, well, it wasn't a conflict lol)

Also, I think that all the translation files should be regenerated at some point, running the extract script locally made a ton of changes everywhere.

Turns out that most/all the other files also had that commented-out section present. Since removing commented-out locale strings isn't relevant to this PR, I reverted that change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants