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 Web Layout #2126

Merged
merged 101 commits into from
Jan 22, 2024
Merged

New Web Layout #2126

merged 101 commits into from
Jan 22, 2024

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Dec 7, 2023

What?

Reworking of web layout that makes the document itself scrollable instead of the list node. Despite the dicey PR title, how the layout looks is meant to remain the same. What I'm aiming to change is how it achieves that look.

Removes virtualization on the web although custom one could plausibly be re-added.

Why?

Fixes scroll jump on clicking into post (before/after):

before_after.mov

Fixes scroll lag on profile:

RPReplay_Final1703215450.MP4

Will also unblock fixing unselectable text in profile header.

Plan

  • Essentials
    • Rip out FlatList which assumes the scrollable node is owned by it
    • Bind onScroll handlers to the window
    • Implement scrollToOffset
    • Make tab headers position: sticky
    • Fix bugs
      • Modal and lightbox can be scrolled away
      • Headers and footers
      • More screen headers need to be changed to fixed
      • What is causing Back to animate?
      • Layout fixes
        • Profile header width at different breakpoints
      • Fix screen backgrounds
        • When cmd+down'ing too hard
        • In light mode during load
      • Safari doesn't trigger "load more"
      • Scroll
        • Move towards a single scroll handler
        • Preserve and reset scroll as needed
          • Figure out why Back resets scroll
          • In pagers, reset and restore position per tab
          • For routes, reset and restore on navigation
            • Proof of concept
            • Polish it up
              • Integrate with pager restore
              • Make sure Back/Forward is sensible
          • Fix post thread jump
  • Perf
    • Add memoization
    • Add containment CSS properties
  • Tidying up
    • Fix native if regressed
    • Clean up the code
    • Fix type errors
    • Check other browsers
    • Check mobile Safari etc
  • Polish
    • Search should jump to top on query change
    • Add scroll lock to modal and lightbox
    • Figure out why iOS Safari lags when scrolling feeds header away
  • Bugs and regressions
    • Search field and pager should be sticky
  • Nitpicks
    • Missing border on Home tablet feeds bar
    • 1px vertical gap between borders on Home tablet feeds bar
    • Empty screen during popstate gesture on iOS Safari (won't fix)
    • Scroll lock mode causes the scrollbar to show/hide which moves content (dropdowns trigger this)
    • When scrollbars are "always on", there's a layout jump on some nav (eg Home -> Notification) on desktop
  • Future follow-ups
    • Fix any types, enforce that .web and .native types match
    • Remove desktopFixedHeight prop if we can (is it needed anymore?)
    • Remove minHeight: Dimensions.get('window').height * 1.5 styles if we can (do we need them anymore?)
    • Get rid of the pointer-events hack to fix unselectable text in profile header
    • Consider deleting FlatList from Views.web and possibly forcing exclusion of its code from the bundle

@gaearon gaearon force-pushed the web-layout branch 13 times, most recently from 8a10c66 to 746443e Compare December 12, 2023 20:59
@pfrazee
Copy link
Collaborator

pfrazee commented Jan 12, 2024

Starting with the positives:

  • Profile header on web is so much smoother
  • Scrolling on web generally feels smoother? maybe placebo effect
  • Text is selectable in profile headers again on web
  • Scroll position dejanked on web when opening replies
  • Scroll wheel on web works no matter where your cursor is!!! ugh god that's nice to have fixed

Small issues that I'd be 100% okay with solving in followups:

  • ProfileList and ProfileScreen on web hit a point (seems like 100% screen height) where the sticky header scrolls away
  • If you go back, forward, then back (via browser tools) you lose your scroll position on the last back. So it's back (scroll position is correct), forward, back (scroll position lost).

Somewhat less minor issues but still okay with putting in followups:

  • Scroll position on web isn't correctly focusing reply posts when I open them from notifications
  • Screens that still need to be updated:
    • Home feed preferences
    • Moderation preferences
    • Saved feeds
    • etc -- just need to do a pass to ensure they're all using the new layout

Copy link
Collaborator

@pfrazee pfrazee left a comment

Choose a reason for hiding this comment

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

This was an enormous PITA slog and I appreciate you sticking with it dan. I'm excited to have this done.

@pfrazee pfrazee merged commit f015229 into main Jan 22, 2024
4 checks passed
@pfrazee pfrazee deleted the web-layout branch January 22, 2024 22:46
estrattonbailey added a commit that referenced this pull request Jan 23, 2024
* origin: (109 commits)
  Update Korean localization (#2494)
  fix: don't expose post content on no-unauthenticated (#2584)
  Tweak positioning
  use color scheme in in-app browser (#2580)
  Fix jump when opening last post in thread (#2591)
  Fix orphan replies in linear mode (#2578)
  Fix typos, improve localization and unify separators (#2502)
  Update Japanese localization based on the latest codebase (#2509)
  Update pt-BR translation (#2471)
  New Web Layout (#2126)
  Update src/locale/locales/id/messages.po
  Update src/locale/locales/id/messages.po
  Update src/locale/locales/id/messages.po
  Update src/locale/locales/id/messages.po
  Update src/locale/locales/id/messages.po
  Update src/locale/locales/id/messages.po
  Update src/locale/locales/id/messages.po
  Update src/locale/locales/id/messages.po
  Update src/locale/locales/id/messages.po
  Update src/locale/locales/id/messages.po
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Not quite a feature but improves existing behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants