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

Refactor sidebar #6971

Merged
merged 17 commits into from
Dec 10, 2024
Merged

Refactor sidebar #6971

merged 17 commits into from
Dec 10, 2024

Conversation

estrattonbailey
Copy link
Member

@estrattonbailey estrattonbailey commented Dec 5, 2024

Extracted from subs branch. This cleans up the right nav to look better and fit incoming NUXs better. Looks especially better on the search page.

In the process, I made an improvement to the new useGutters hook so it's more ergonomic.

CleanShot 2024-12-05 at 19 56 23@2x
CleanShot 2024-12-05 at 19 56 34@2x
CleanShot 2024-12-05 at 19 56 38@2x
CleanShot 2024-12-05 at 19 57 19@2x
CleanShot 2024-12-05 at 20 11 02@2x
(illustrating overflow with lots of feeds, works fine)
CleanShot 2024-12-05 at 20 15 23@2x

Copy link

github-actions bot commented Dec 5, 2024

Old size New size Diff
7.74 MB 7.74 MB -625 B (-0.01%)

@arcalinea arcalinea temporarily deployed to new-sidebar - social-app PR #6971 December 6, 2024 01:58 — with Render Destroyed
@arcalinea arcalinea temporarily deployed to new-sidebar - social-app PR #6971 December 6, 2024 02:08 — with Render Destroyed
@arcalinea arcalinea temporarily deployed to new-sidebar - social-app PR #6971 December 6, 2024 02:09 — with Render Destroyed
@@ -34,7 +34,7 @@ export function Outer({
noBottomBorder?: boolean
}) {
const t = useTheme()
const gutter = useGutterStyles()
const gutters = useGutters([0, 'base'])
Copy link
Member Author

Choose a reason for hiding this comment

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

No change here from previous styles

@@ -41,14 +41,14 @@ function HomeHeaderLayoutDesktopAndTablet({
const {hasSession} = useSession()
const {_} = useLingui()
const kawaii = useKawaiiMode()
const gutter = useGutterStyles()
const gutters = useGutters([0, 'base'])
Copy link
Member Author

Choose a reason for hiding this comment

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

No change here from previous styles

@Tanza3D
Copy link
Contributor

Tanza3D commented Dec 6, 2024

image
these feel reaaally squished together now

@estrattonbailey
Copy link
Member Author

@Tanza3D I think you may be looking at an old version here. It's missing a link at the bottom for "More feeds" and the spacing around the sidebar is now wider on the left than on top, whereas here it looks event.

Or are you only referring to the line-height between the links?

@Tanza3D
Copy link
Contributor

Tanza3D commented Dec 6, 2024

@Tanza3D I think you may be looking at an old version here. It's missing a link at the bottom for "More feeds" and the spacing around the sidebar is now wider on the left than on top, whereas here it looks event.

Or are you only referring to the line-height between the links?

mainly referring to the line height/padding between the feed links/buttons yeah; I feel they'd be better displayed as tabs similar to discords channel lists and such perhaps

src/alf/util/useGutters.ts Outdated Show resolved Hide resolved
src/alf/util/useGutters.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

thanks! a few things that jumped out to me that seem like regressions:

  • pressing on a feed has gotten much slower in this PR, something regressed there
  • feed link staying underlined after navigation click feels weird to me, not sure why it does that
  • something's wrong with the top bar, the butterfly and the # icon lost top padding

these are maybe more subjective:

  • not sure about more narrow input, it's very noticeable how usernames rarely fit and display names don't fit more often too. i would suggest that if we were to narrow it, we should also turn the dropdown into an actual dropdown (and give more width to the list)
  • not a fan of "external link" icon on more feeds, it looks like a link to another site
  • overall they feel a bit too crowded to me, this is secondary navigation but they're very close to each other and easy to misclick. i'd prefer if there was a bit more space there

lmk if you disagree with some of these

@arcalinea arcalinea temporarily deployed to new-sidebar - social-app PR #6971 December 10, 2024 20:16 — with Render Destroyed
@arcalinea arcalinea temporarily deployed to new-sidebar - social-app PR #6971 December 10, 2024 20:18 — with Render Destroyed
@arcalinea arcalinea temporarily deployed to new-sidebar - social-app PR #6971 December 10, 2024 20:22 — with Render Destroyed
Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

lfg

@gaearon gaearon merged commit e052f5e into main Dec 10, 2024
6 checks passed
@gaearon gaearon deleted the new-sidebar branch December 10, 2024 20:52
<Trans>
Logo by{' '}
<InlineLinkText
label={_(msg`Logo by @sawaratsuki.bsky.social`)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we translate @sawaratsuki.bsky.social? If not, what about using a placeholder instead of that username?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah there’s also already “logo by” around it, I think we can just remove the repetition to what it was before.

Signez pushed a commit to Signez/bsky-social-app that referenced this pull request Dec 26, 2024
* Refactor RightNav

(cherry picked from commit 96bb02a)

* Better gutter handling

* Clean up styles

* Memoize breakpoints

* Format

* Comment

* Loosen spacing, handle overflow, smaller text to match prod

* Fix circular imports on native

* Return 0 instead of undefined for easier calculations

* Re-assign

* Fix

* Port over fix from subs/base

* Space out right nav feeds, widen sidebar to match prod

* Fix lost padding on home header

* Fix perf by not actually linking to new URL

* Remove underline on focus

* Foramt

---------

Co-authored-by: Dan Abramov <[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.

5 participants