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

Pinned feeds cards #4526

Merged
merged 15 commits into from
Jun 21, 2024
Merged

Pinned feeds cards #4526

merged 15 commits into from
Jun 21, 2024

Conversation

estrattonbailey
Copy link
Member

@estrattonbailey estrattonbailey commented Jun 14, 2024

Replaces the lil avi+feed name at the top of the Feeds screen. Adds support for lists in FeedCard. Fixes alignment on mobile.

One small improvement from prod is that there's a pressed state on native now, that matches the hover on web.

CleanShot 2024-06-17 at 14 53 20@2x
CleanShot 2024-06-17 at 14 53 00@2x

Copy link

render bot commented Jun 14, 2024

Copy link

github-actions bot commented Jun 14, 2024

Old size New size Diff
6.31 MB 6.31 MB 2.23 KB (0.03%)

@estrattonbailey estrattonbailey marked this pull request as ready for review June 17, 2024 19:55
* origin/main: (22 commits)
  Disable newskie dialog tap in hover card web (#4562)
  Implement thread locking (#4545)
  Prevent unecessary calls (#4561)
  Force callers of `getTimeAgo` to pass in the value for "now" (#4560)
  Fix: only apply self-thread load-more behavior on the outer edge of the reply tree (#4559)
  Server-side thread mutes (#4518)
  Explore fixes (#4540)
  Is it "newskie" or "newsky" 🤔  (#4557)
  fix keyboard overlaying onboarding inputs (#4558)
  Add `useGetTimeAgo` and utils (#4556)
  Unconditionally polyfill Intl.PluralRules for native (#4554)
  Dedupe Zod installation (#4551)
  Use exact imports for icons (#4549)
  Fix Android startup perf regression (#4544)
  Explore feed cards (#4521)
  Onboarding fixes (#4508)
  Add `native_pwi_disabled` feature gate experiment (#4507)
  Select, don't mutate (#4541)
  Don't show "Pin/Add" button on feed card w/ no session (#4539)
  Add patch for `RCTBaseTextInput` fixing `selectTextOnFocus` prop (#4533)
  ...
@gaearon
Copy link
Collaborator

gaearon commented Jun 19, 2024

this loading sequence feels a bit janky. some ideas:

  1. use a glimmer for the top section too
  2. maybe don't show the bottom section until the top one has loaded — it's just distracting that it'll jump soon
loading.mov

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.

let's polish the loading sequence a bit

@estrattonbailey
Copy link
Member Author

Oh yep, meant to do this, I have a good idea for how to do this nicely I think

@pfrazee pfrazee assigned pfrazee and unassigned pfrazee Jun 20, 2024
* origin/main: (25 commits)
  Add a11y context (#4586)
  center pill text in label pill (#4579)
  Wait for AppView when posting (#4584)
  Bsky link card service (#4547)
  Merge #4492, fixes profile menu hover (#4580)
  Rework "Who can reply" to blend more nicely into the UI (#4578)
  Fix threadgate read after write (#4577)
  Convert button to use forwardRef (#4576)
  use 1000x1000 for image height in avatar cropper (#4453)
  fix for autofill covering border (#4573)
  Update HomeHeaderLayoutMobile.tsx (#4572)
  Option for large alt badges (#4571)
  Truncate post metrics and fix truncation on native (#4575)
  Fix avi placeholder layout (#4570)
  add support for `ListEmptyComponent`, allow `undefined` data (#4403)
  GIF previews in notifications (#4447)
  [Session] Convert account to session data explicitly (#4446)
  Move onboarding start to after successfull account creation (#4381)
  Collection of moderation fixes (#4566)
  Fix undefined block (#4479)
  ...
@estrattonbailey
Copy link
Member Author

Added a skeleton loading state.

Cool thing we can do here is load the exact # of placeholders we need, bc we should have preferences loaded and therefore know how many saved feeds the user has.

RQ has a placeholderData param that we can use to provide initial data, and a isPlaceholderData state we can use in place of isLoading.

On warm loads i.e. navigation, we have prefs loaded so we can smoothly show placeholder -> real cards. On cold loads, prefs haven't loaded yet, so even the placeholderData will have a count === 0. In this case, we show 4 placeholders.

I double checked this in a no-feeds state, which can only happen if the user rugs their feeds in a 3p app or directly via API access, and the logic still works as it has been.

@estrattonbailey
Copy link
Member Author

And pushed a fix for a flash of the load state when pinning. RQ let's us re-use the previous query result as the next placeholder state when the query is invalidated after a pin.

@gaearon
Copy link
Collaborator

gaearon commented Jun 21, 2024

jump.mov

The initial load is still jumpy because we're showing both sections despite not knowing the height of the first section. So once the first section arrives, it re-layouts. Two questions:

  • Can we show My Feeds and the My Feeds glimmer alone (without Discover and Discover glimmer) until the query below has loaded? Similar to how we load the profile screen top-to-bottom (even if we have the bottom data earlier). This would avoid the layout jump.
  • What is the reason for the second glimmer -> content transition? Meaning the transition from when I see a correctly-sized glimmer (presumably by this point we know the number of pinned feeds) to when I see all content. How come we know the number of My Feeds but not their content? Intuitively I'd expect us to know both from a single request.

To sum up, I'd expect this screen to load in two stages:

  1. The first stage is My Feeds and a generic glimmer. No bottom section if the top section isn't ready.
  2. The second stage is either all content being ready or My Feeds content being ready (with Discover still loading).

@estrattonbailey
Copy link
Member Author

Yeah good idea. The goal of this PR isn't to totally redo this page, I just want to get rid of the n+1 queries we're doing for each saved feed and all the feeds under the bottom section (that bit I did last week).

I think I explained the multiple glitters above. On cold loads it's due to preferences not having been loaded yet. But I could see an argument for not jumping between the base of 4, to the actual # of feeds, to the loaded feeds. We could just show 4 or so and then wait for feeds data and show all feeds fully loaded.

Keep in mind that's only on cold loads though. With warm preferences data, I think it's super nice.

I'll look at the staged loading you're suggesting and see if it's easy to hit.

@gaearon
Copy link
Collaborator

gaearon commented Jun 21, 2024

Would be good to get this fixed:

regr.mov

Feels disorienting. Should doable by implementing placeholderData that looks at saved feeds + suggested feeds queries.

@gaearon gaearon merged commit 4d67870 into main Jun 21, 2024
6 checks passed
@gaearon gaearon deleted the pinned-feeds-cards branch June 21, 2024 21:50
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