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

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

Merged
merged 5 commits into from
May 20, 2024

Conversation

r1tsuu
Copy link
Contributor

@r1tsuu r1tsuu commented May 4, 2024

Description

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

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

Type of change

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

Checklist:

  • Existing test suite passes locally with my changes

@jacobsfletch
Copy link
Member

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.

@r1tsuu
Copy link
Contributor Author

r1tsuu commented May 7, 2024

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.

@JessChowdhury
Copy link
Member

  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?

@jmikrut
Copy link
Member

jmikrut commented May 20, 2024

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!

Copy link
Member

@jacobsfletch jacobsfletch left a comment

Choose a reason for hiding this comment

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

Looks like this is actually causing some failing tests related to recursive dependencies. Will need to fix this before merging.

@jmikrut jmikrut merged commit e682cb1 into payloadcms:beta May 20, 2024
32 of 34 checks passed
@jmikrut
Copy link
Member

jmikrut commented May 20, 2024

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

@r1tsuu
Copy link
Contributor Author

r1tsuu commented May 21, 2024

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

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.

state of relationship Cell component on the List View after changing page remains the same
4 participants