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

Relationship select fixes #6839

Merged
merged 8 commits into from
Oct 28, 2021
Merged

Relationship select fixes #6839

merged 8 commits into from
Oct 28, 2021

Conversation

emmatown
Copy link
Member

Closes #6822
Closes #6823

@changeset-bot
Copy link

changeset-bot bot commented Oct 27, 2021

🦋 Changeset detected

Latest commit: 09dcceb

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@keystone-next/keystone Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link

vercel bot commented Oct 27, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/keystonejs/keystone-next-docs/CetaHHFoWZrM8XxC5s8cC9E3AUYM
✅ Preview: https://keystone-next-docs-git-relationship-select-fixes-keystonejs.vercel.app

@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 27, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@@ -1,6 +1,6 @@
import * as Path from 'path';
import { GraphQLSchema } from 'graphql';
import type { AdminMetaRootVal, KeystoneConfig, AdminFileToWrite } from '../../../types';
import type { AdminMetaRootVal, KeystoneConfig, AdminFileToWrite } from '../../types';
Copy link
Member Author

@emmatown emmatown Oct 27, 2021

Choose a reason for hiding this comment

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

This and the change in the file below is unrelated to the rest of this, it's just a thing I noticed after doing a build and running tsc because the previous path points to the dist location of that entrypoint rather than src location which it should point to which makes this point to nowhere in the .d.ts files, probably gonna add something to prevent this sort of problem soon.

@emmatown emmatown requested a review from a team October 27, 2021 03:32
@JedWatson JedWatson added this to the 2021 October 2H Release milestone Oct 27, 2021
@@ -39,12 +64,15 @@ function useFilter(search: string, list: ListMeta) {
const trimmedSearch = search.trim();
const isValidId = idValidators[idFieldKind](trimmedSearch);
if (isValidId) {
conditions.push({ id: trimmedSearch });
conditions.push({ id: { equals: trimmedSearch } });
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the fix for #6823, we were passing a bad filter previously, try this by putting in an id of an item and that item should be in the options.

Comment on lines +72 to +75
[field.path]: {
contains: trimmedSearch,
mode: field.search === 'insensitive' ? 'insensitive' : undefined,
},
Copy link
Member Author

Choose a reason for hiding this comment

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

This in combination with the changes to createAdminMeta mean that what you type in the select will actually filter the options again assuming the ui.labelField is filterable or you've configured ui.searchFields so try searching for an item and you should only see items that match the search.

@@ -120,11 +148,43 @@ export const RelationshipSelect = ({
}
`;

const where = useFilter(search, list);
const debouncedSearch = useDebouncedValue(search, 200);
Copy link
Member Author

Choose a reason for hiding this comment

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

This means till you're done typing before fetching so you shouldn't see a bunch of network requests when you're typing

Comment on lines +207 to +228
// we want to avoid fetching more again and `loading` from Apollo
// doesn't seem to become true when fetching more
const [lastFetchMore, setLastFetchMore] = useState<{
where: Record<string, any>;
extraSelection: string;
list: ListMeta;
skip: number;
} | null>(null);

useIntersectionObserver(
([{ isIntersecting }]) => {
if (!loading && isIntersecting && options.length < count) {
const skip = data?.items.length;
if (
!loading &&
skip &&
isIntersecting &&
options.length < count &&
(lastFetchMore?.extraSelection !== extraSelection ||
lastFetchMore?.where !== where ||
lastFetchMore?.list !== list ||
lastFetchMore?.skip !== skip)
) {
Copy link
Member Author

Choose a reason for hiding this comment

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

All this and the stuff below is to avoid fetching more when we're already fetching more, previously if you scrolled to the bottom and then scrolled up a bit so you couldn't see the thing that said loading and then scrolled down it would fetch again if the previous request was still in progress.

Comment on lines 161 to 179
cache: new InMemoryCache({
typePolicies: {
Query: {
fields: {
[list.gqlNames.listQueryName]: {
keyArgs: ['where'],
merge: (existing: readonly any[], incoming: readonly any[], { args }) => {
const merged = existing ? existing.slice() : [];
const { skip } = args!;
for (let i = 0; i < incoming.length; ++i) {
merged[skip + i] = incoming[i];
}
return merged;
},
},
},
},
},
}),
Copy link
Member Author

Choose a reason for hiding this comment

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

This fixes the duplicate items thing because we're assigning an item to the position based on the skip of the request rather than blindly concatenating the responses together

Copy link
Member

Choose a reason for hiding this comment

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

Can we use unknown instead of any?

@vercel vercel bot temporarily deployed to Preview October 28, 2021 00:12 Inactive
@vercel vercel bot temporarily deployed to Preview October 28, 2021 00:41 Inactive
Copy link
Contributor

@bladey bladey left a comment

Choose a reason for hiding this comment

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

Works well.

@vercel vercel bot temporarily deployed to Preview October 28, 2021 00:58 Inactive
@emmatown emmatown enabled auto-merge (squash) October 28, 2021 00:58
@emmatown emmatown merged commit d1141ea into main Oct 28, 2021
@emmatown emmatown deleted the relationship-select-fixes branch October 28, 2021 01:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants