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

Merged
merged 18 commits into from
Dec 12, 2024
Merged

New profile feed header #7056

merged 18 commits into from
Dec 12, 2024

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

github-actions bot commented Dec 11, 2024

Old size New size Diff
6.77 MB 6.78 MB 5.02 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 👀

src/screens/Profile/components/ProfileFeedHeader.tsx Outdated Show resolved Hide resolved
<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? 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, Trans should be pulled outside here

@arcalinea arcalinea temporarily deployed to new-profile-feed-header - social-app PR #7056 December 12, 2024 17:38 — with Render Destroyed
@arcalinea arcalinea temporarily deployed to new-profile-feed-header - social-app PR #7056 December 12, 2024 17:41 — with Render Destroyed
@arcalinea arcalinea temporarily deployed to new-profile-feed-header - social-app PR #7056 December 12, 2024 17:45 — with Render Destroyed
@estrattonbailey estrattonbailey merged commit 2808f8b into main Dec 12, 2024
6 checks passed
Signez pushed a commit to Signez/bsky-social-app that referenced this pull request Dec 26, 2024
* Init hacking

* Lil baby button checkpoint

* Playing around

* Revert "Playing around"

This reverts commit f58a7fa.

* Mostly there

* Cleanups

* Cleanup

* Fix report dialog nesting

* Remove transform on native

* Rename header

* Fix layout, overflowing FAB buttons

* Remove hack

* Couple of fixes

* Keep Pin primary CTA (bluesky-social#7061)

* Update src/screens/Profile/components/ProfileFeedHeader.tsx

Co-authored-by: surfdude29 <[email protected]>

* Simplify, use old string

* Wrap Trans better

---------

Co-authored-by: dan <[email protected]>
Co-authored-by: surfdude29 <[email protected]>
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