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

New profile feed header #7056

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

New profile feed header #7056

wants to merge 14 commits into from

Conversation

estrattonbailey
Copy link
Member

@estrattonbailey estrattonbailey commented Dec 11, 2024

Medium-sized step towards some cleaner UI for this screen. More work will be done here.

This PR aims to keep translation strings mostly the same, but some were added.

CleanShot 2024-12-11 at 14 27 53@2x
CleanShot 2024-12-11 at 14 27 56@2x
CleanShot 2024-12-11 at 14 28 01@2x
CleanShot 2024-12-11 at 14 28 03@2x

@arcalinea arcalinea temporarily deployed to new-profile-feed-header - social-app PR #7056 December 11, 2024 20:27 — with Render Destroyed
Copy link

Old size New size Diff
6.77 MB 6.77 MB 4.83 KB (0.07%)

Copy link
Contributor

@surfdude29 surfdude29 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of tiny suggestions 👀

if (pinned) {
Toast.show(_(msg`Pinned ${info.displayName} to Home`))
} else {
Toast.show(_(msg`Un-pinned ${info.displayName} from Home`))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Toast.show(_(msg`Un-pinned ${info.displayName} from Home`))
Toast.show(_(msg`Unpinned ${info.displayName} from Home`))

I think it would work better without the hyphen, as currently all the unpin* strings in the app don't use one

<View
style={[a.flex_row, a.align_center, a.gap_sm, a.justify_between]}>
<Text style={[a.italic, t.atoms.text_contrast_medium]}>
Something wrong? Let us know.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Something wrong? Let us know.
<Trans>Something wrong? Let us know.</Trans>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, just wondering, will the text be able to wrap onto another line here at larger text sizes and in non-English localizations?

<Text
style={[a.text_sm, a.leading_tight, t.atoms.text_contrast_medium]}
numberOfLines={1}>
<Trans>By</Trans>{' '}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I'm wondering if it might be desirable (or indeed feasible) to restructure this, so that By and the handle are both in the string to be translated, for languages where some flexibility is needed? 🤔

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.

4 participants