payload icon indicating copy to clipboard operation
payload copied to clipboard

fix(ui): update relationship cell formatted value when when search changes

Open r1tsuu opened this issue 9 months ago • 3 comments

Description

Fixes https://github.com/payloadcms/payload-3.0-demo/issues/181 Although issue is about page changing, it happens as well when you change sort / limit / where filter (and probably locale)

  • [x] I have read and understand the CONTRIBUTING.md document in this repository.

Type of change

  • [x] Bug fix (non-breaking change which fixes an issue)

Checklist:

  • [x] Existing test suite passes locally with my changes

r1tsuu avatar May 04 '24 04:05 r1tsuu

This needs a test as well, elsewhere in our e2e test suites (probably Live Preview) Playwright is able to "watch" the network requests being made. That's what we need to do here for two tests at least, 1. to test that changing results (i.e. a new page) triggers new network requests (and updates the cell's HTML), and 2. to test that changing params but not results does not trigger a network request.

jacobsfletch avatar May 07 '24 15:05 jacobsfletch

Sorry, i could be wrong, but did you guys also consider to populate these server side, since we anyway are retrieving docs in the server component? We still want depth: 0 of course, but populate manually with our dataLoader user's selected relationship columns / top level relations.

that's also debatable by UX i think, some people prefer this lazy loading thing and some don't want to see that something is still loading but don't might if the request would took a little bit longer.

But of course if a bottleneck here is huge, then i'm totally wrong.

r1tsuu avatar May 07 '24 17:05 r1tsuu

  1. to test that changing results (i.e. a new page) triggers new network requests (and updates the cell's HTML), and 2. to test that changing params but not results does not trigger a network request.

@jacobsfletch I've added the first test - for 2. when you say changing the params but not the results, what do you mean?

JessChowdhury avatar May 08 '24 14:05 JessChowdhury

Sorry, i could be wrong, but did you guys also consider to populate these server side, since we anyway are retrieving docs in the server component?

Hey @r1tsuu yep in a much earlier version of Payload we did indeed load all relations via the dataloader, but the problem was when you have long pages with many rows on the screen at once (100+). The response was just huge.

Keeping the initial load down as tiny as possible makes sure that the initial load is speedy all the time, and then we can lazy-load relations when they scroll onto the screen. But it's a good thought!

I think this PR is good to go at this point, right? @jacobsfletch you wanna merge and we can get it out in a new beta?

Thank you everyone!

jmikrut avatar May 20 '24 19:05 jmikrut

i'm on the fix for that right now - thanks for the help here everyone!

jmikrut avatar May 20 '24 20:05 jmikrut

Thanks @jacobsfletch @JessChowdhury for carrying here and @jmikrut for bumping this!

r1tsuu avatar May 21 '24 02:05 r1tsuu