Skip to content
This repository has been archived by the owner on Apr 25, 2023. It is now read-only.

refactor: refactor the infinite scroll wrapper and queries #267

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

refactor: refactor the infinite scroll wrapper and queries #267

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions src/components/atoms/InfiniteScroll/hooks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import { MutableRefObject } from "react";

export default (onLoadMore?: () => void) => {
const handleAutoFillPage = (ref: MutableRefObject<HTMLDivElement | undefined | null>) => {
if (ref.current && ref.current?.scrollHeight <= document.documentElement.clientHeight) {
onLoadMore?.();
}
Comment on lines +5 to +7
Copy link
Member

Choose a reason for hiding this comment

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

I think something about this isn't quite working correctly. In the Asset settings page you get this called when the page loads, before any scrolling. It isn't a BIG deal, since it only seems to call once, but it really shouldn't call UNLESS the page is too long for the initial fetch.

Copy link
Contributor

@airslice airslice Jul 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Just happen to see this. I think this part comes from my suggestion some weeks ago. This is only for solving large screen issue (first fetch cannot fill the area) and yes it should be called only under this situation but the point is it seems to be some difficult to check if it is too long (from my memory it was due to the DOM structure is different with different usecases and the function is a common util at that time). So my idea was always want the content fill the area and if not and if there's more data then load more. At that time i only look into this with load projects in large screen issue and i'm not sure about other situations (like Asset page). And also i think the code changes a lot now maybe can optimize this with adding an area's ref?

};

const handleScrollToBottom = (
{ currentTarget }: React.UIEvent<HTMLDivElement, UIEvent>,
threshold = 5,
) => {
if (
currentTarget.scrollTop + currentTarget.clientHeight >=
currentTarget.scrollHeight - threshold
) {
onLoadMore?.();
}
};

return { handleAutoFillPage, handleScrollToBottom };
};
44 changes: 44 additions & 0 deletions src/components/atoms/InfiniteScroll/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
import React, { ReactNode, useEffect, useRef } from "react";

import { styled } from "@reearth/theme";

import useHooks from "./hooks";

type Props = {
className?: string;
loading?: boolean;
hasMoreItems?: boolean;
children?: ReactNode;
onGetMoreItems?: () => void;
};

const InfiniteScrollWrapper: React.FC<Props> = ({
className,
loading,
hasMoreItems,
onGetMoreItems,
children,
}) => {
const wrapperRef = useRef<HTMLDivElement>(null);
const { handleAutoFillPage, handleScrollToBottom } = useHooks(onGetMoreItems);

useEffect(() => {
if (wrapperRef.current && !loading && hasMoreItems) handleAutoFillPage(wrapperRef);
}, [handleAutoFillPage, hasMoreItems, loading, onGetMoreItems]);
Comment on lines +22 to +27
Copy link
Member

Choose a reason for hiding this comment

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

This component doesn't have too many hooks, so you don't really need the hooks.ts file. But if you want to keep the file, please bring the ref and useEffect into the hooks file as well to keep things consistent (unless there is a reason you separated them, in which case please let me know).


return (
<Wrapper
ref={wrapperRef}
className={className}
onScroll={e => !loading && hasMoreItems && handleScrollToBottom(e)}>
{children}
</Wrapper>
);
};
const Wrapper = styled.div`
width: 100%;
height: 100%;
overflow-y: scroll;
-webkit-overflow-scrolling: touch;
`;
export default React.memo(InfiniteScrollWrapper);
17 changes: 5 additions & 12 deletions src/components/molecules/Common/AssetModal/AssetContainer/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,15 @@
import { useEffect, useRef } from "react";

import Button from "@reearth/components/atoms/Button";
import Divider from "@reearth/components/atoms/Divider";
import Flex from "@reearth/components/atoms/Flex";
import Icon from "@reearth/components/atoms/Icon";
import InfiniteScrollWrapper from "@reearth/components/atoms/InfiniteScroll";
import Loading from "@reearth/components/atoms/Loading";
import SearchBar from "@reearth/components/atoms/SearchBar";
import Text from "@reearth/components/atoms/Text";
import AssetDeleteModal from "@reearth/components/molecules/Common/AssetModal/AssetDeleteModal";
import { useT } from "@reearth/i18n";
import { styled } from "@reearth/theme";
import { metricsSizes } from "@reearth/theme/metrics";
import { autoFillPage, onScrollToBottom } from "@reearth/util/infinite-scroll";

import AssetCard from "../AssetCard";
import AssetListItem from "../AssetListItem";
Expand Down Expand Up @@ -100,11 +98,6 @@ const AssetContainer: React.FC<Props> = ({
onRemove,
onSearch,
});
const wrapperRef = useRef<HTMLDivElement>(null);

useEffect(() => {
if (wrapperRef.current && !isLoading && hasMoreAssets) autoFillPage(wrapperRef, onGetMore);
}, [hasMoreAssets, isLoading, onGetMore]);

return (
<Wrapper>
Expand Down Expand Up @@ -175,8 +168,9 @@ const AssetContainer: React.FC<Props> = ({
</Template>
) : (
<AssetListWrapper
ref={wrapperRef}
onScroll={e => !isLoading && hasMoreAssets && onScrollToBottom(e, onGetMore)}>
loading={isLoading}
hasMoreItems={hasMoreAssets}
onGetMoreItems={onGetMore}>
<AssetList layoutType={layoutType}>
{layoutType === "list"
? assets?.map(a => (
Expand Down Expand Up @@ -244,10 +238,9 @@ const AssetWrapper = styled.div<{ height?: number }>`
justify-content: space-between;
`;

const AssetListWrapper = styled.div`
const AssetListWrapper = styled(InfiniteScrollWrapper)`
display: flex;
flex-direction: column;
overflow-y: scroll;
scrollbar-width: none;
&::-webkit-scrollbar {
display: none;
Expand Down
22 changes: 6 additions & 16 deletions src/components/molecules/Dashboard/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import { ReactNode, useEffect, useRef } from "react";
import { ReactNode } from "react";

import InfiniteScrollWrapper from "@reearth/components/atoms/InfiniteScroll";
import Loading from "@reearth/components/atoms/Loading";
import { styled } from "@reearth/theme";
import { autoFillPage, onScrollToBottom } from "@reearth/util/infinite-scroll";

export * from "./types";

Expand All @@ -23,20 +23,12 @@ const Dashboard: React.FC<Props> = ({
hasMoreProjects,
onGetMoreProjects,
}) => {
const wrapperRef = useRef<HTMLDivElement>(null);

useEffect(() => {
if (wrapperRef.current && !isLoading && hasMoreProjects)
autoFillPage(wrapperRef, onGetMoreProjects);
}, [hasMoreProjects, isLoading, onGetMoreProjects]);

return (
<Wrapper
className={className}
ref={wrapperRef}
onScroll={e => {
!isLoading && hasMoreProjects && onScrollToBottom(e, onGetMoreProjects);
}}>
loading={isLoading}
hasMoreItems={hasMoreProjects}
onGetMoreItems={onGetMoreProjects}>
{header}
<Content>
{children}
Expand All @@ -46,10 +38,8 @@ const Dashboard: React.FC<Props> = ({
);
};

const Wrapper = styled.div`
const Wrapper = styled(InfiniteScrollWrapper)`
background: ${({ theme }) => theme.dashboard.bg};
height: 100%;
overflow: auto;
`;

const Content = styled.div`
Expand Down
4 changes: 2 additions & 2 deletions src/components/molecules/Settings/Project/Dataset/DatasetList/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ export type Item = {
};

export type Props = {
items: Item[];
items?: Item[];
isLoading?: boolean;
hasMore?: boolean;
removeDatasetSchema?: (schemaId: string) => void;
Expand All @@ -19,7 +19,7 @@ export type Props = {
const DatasetList: React.FC<Props> = ({ items, removeDatasetSchema }) => {
return (
<Wrapper>
{items.map(item => (
{items?.map(item => (
<DatasetItem key={item.id} {...item} removeDatasetSchema={removeDatasetSchema} />
))}
</Wrapper>
Expand Down
2 changes: 1 addition & 1 deletion src/components/molecules/Settings/Project/Dataset/DatasetSection/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { useT } from "@reearth/i18n";
import { styled } from "@reearth/theme";

type Props = {
datasetSchemas: Item[];
datasetSchemas?: Item[];
removeDatasetSchema: (schemaId: string) => void;
onDatasetImport?: (file: File, datasetSchemaId: string | null) => void | Promise<void>;
};
Expand Down
20 changes: 4 additions & 16 deletions src/components/molecules/Settings/SettingPage/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
import { useEffect, useRef, useState } from "react";
import { useState } from "react";
import { Link } from "react-router-dom";

import Icon from "@reearth/components/atoms/Icon";
import InfiniteScrollWrapper from "@reearth/components/atoms/InfiniteScroll";
import Loading from "@reearth/components/atoms/Loading";
import Header, { Props } from "@reearth/components/molecules/Common/Header";
import ProjectMenu from "@reearth/components/molecules/Common/ProjectMenu";
import Navigation from "@reearth/components/molecules/Settings/Navigation";
import { styled } from "@reearth/theme";
import { autoFillPage, onScrollToBottom } from "@reearth/util/infinite-scroll";

export type SettingPageProps = {
loading?: boolean;
Expand All @@ -30,12 +30,6 @@ const SettingPage: React.FC<SettingPageProps> = ({
setIsOpen(o => !o);
};

const wrapperRef = useRef<HTMLDivElement>(null);

useEffect(() => {
if (wrapperRef.current && !loading && hasMoreItems) autoFillPage(wrapperRef, onScroll);
}, [hasMoreItems, loading, onScroll]);

return (
<Wrapper>
<StyledHeader
Expand All @@ -52,11 +46,7 @@ const SettingPage: React.FC<SettingPageProps> = ({
currentProject && <ProjectMenu currentProject={currentProject} teamId={currentTeam?.id} />
}
/>
<BodyWrapper
ref={wrapperRef}
onScroll={e => {
!loading && hasMoreItems && onScrollToBottom(e, onScroll);
}}>
<BodyWrapper loading={loading} hasMoreItems={hasMoreItems} onGetMoreItems={onScroll}>
<LeftWrapper>
<Navigation team={currentTeam} project={currentProject} />
</LeftWrapper>
Expand Down Expand Up @@ -96,10 +86,8 @@ const Wrapper = styled.div`
flex-direction: column;
`;

const BodyWrapper = styled.div`
height: 100%;
const BodyWrapper = styled(InfiniteScrollWrapper)`
padding-top: 48px;
overflow-y: scroll;
`;

const LeftWrapper = styled.div`
Expand Down
19 changes: 15 additions & 4 deletions src/components/organisms/Common/AssetContainer/hooks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import { useApolloClient } from "@apollo/client";
import { useCallback, useState, useEffect } from "react";

import {
GetAssetsQuery,
useCreateAssetMutation,
useRemoveAssetMutation,
useGetAssetsQuery,
Expand All @@ -12,8 +11,6 @@ import {
import { useT } from "@reearth/i18n";
import { useNotification } from "@reearth/state";

export type AssetNodes = NonNullable<GetAssetsQuery["assets"]["nodes"][number]>[];

export type AssetSortType = "date" | "name" | "size";

export type Asset = {
Expand Down Expand Up @@ -74,7 +71,21 @@ export default (teamId?: string, initialAssetUrl?: string | null, allowDeletion?
data?.assets.pageInfo?.hasNextPage || data?.assets.pageInfo?.hasPreviousPage;

const isRefetching = networkStatus === 3;
const assets = data?.assets.edges?.map(e => e.node) as AssetNodes;

const assets = data?.assets.edges
.map<Asset | undefined>(asset =>
asset.node
? {
id: asset.node.id,
name: asset.node.name,
size: asset.node.size,
teamId: asset.node.teamId,
url: asset.node.url,
contentType: asset.node.contentType,
}
: undefined,
)
.filter((asset): asset is Asset => !!asset);
Comment on lines +75 to +88
Copy link
Member

Choose a reason for hiding this comment

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

I think you can keep it more concise if you do something like this:

const assets = data?.assets.edges
    .map<Asset | undefined>(({ node })=> node)
    .filter((asset): asset is Asset => !!asset);

Please test to make sure it works though.
Also, what is the purpose of the filter? Is there any chance that there'll be an undefined asset inside the assets array?


const initialAsset = assets?.find(a => a.url === initialAssetUrl);
const [selectedAssets, selectAsset] = useState<Asset[]>(initialAsset ? [initialAsset] : []);
Expand Down
25 changes: 10 additions & 15 deletions src/components/organisms/Dashboard/hooks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,10 @@ import {
useCreateProjectMutation,
useCreateSceneMutation,
Visualizer,
GetProjectsQuery,
} from "@reearth/gql";
import { useT } from "@reearth/i18n";
import { useTeam, useProject, useUnselectProject, useNotification } from "@reearth/state";

export type ProjectNodes = NonNullable<GetProjectsQuery["projects"]["nodes"][number]>[];

const projectsPerPage = 9;

export default (teamId?: string) => {
Expand Down Expand Up @@ -117,25 +114,23 @@ export default (teamId?: string) => {
notifyOnNetworkStatusChange: true,
});

const projectNodes = projectData?.projects.edges.map(e => e.node) as ProjectNodes;

const projects = useMemo(() => {
return (projectNodes ?? [])
return (projectData?.projects.edges ?? [])
.map<Project | undefined>(project =>
project
project.node
? {
id: project.id,
description: project.description,
name: project.name,
image: project.imageUrl,
status: toPublishmentStatus(project.publishmentStatus),
isArchived: project.isArchived,
sceneId: project.scene?.id,
id: project.node.id,
description: project.node.description,
name: project.node.name,
image: project.node.imageUrl,
status: toPublishmentStatus(project.node.publishmentStatus),
isArchived: project.node.isArchived,
sceneId: project.node.scene?.id,
}
: undefined,
)
.filter((project): project is Project => !!project);
}, [projectNodes]);
}, [projectData?.projects.edges]);
Comment on lines 117 to +133
Copy link
Member

Choose a reason for hiding this comment

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

Similar to the comment above, you can destructure the node so you can clean up the assignment below, something like this:

 const projects = useMemo(() => {
    return (projectData?.projects.edges ?? [])
      .map<Project | undefined>(({node})=>
           node
          ? {
              id: node.id,
              description: node.description,
              name: node.name,
              image: node.imageUrl,
              status: toPublishmentStatus(node.publishmentStatus),
              isArchived: node.isArchived,
              sceneId: node.scene?.id,
            }
          : undefined,
      )
      .filter((project): project is Project => !!project);
  }, [projectData?.projects.edges]);

You can also destructure further like this:

(projectData?.projects.edges ?? [])
      .map<Project | undefined>(({node: { id, description, name, imageUrl: image, publishmentStatus: status, isArchived, scene: {id?: sceneId}}})=> {
   id,
   description,
   name,
   etc, 
   etc,
}

The second one might be too much? Also, you can go even further to ONLLY destructure specifically the keys that are different between the data and the frontend's Project (aka image VS imageUrl, etc) and leave the rest as ...rest. But again, it might be too much and the code might become harder to understand. I'll let you play around and find whats best here


const hasMoreProjects =
projectData?.projects.pageInfo?.hasNextPage || projectData?.projects.pageInfo?.hasPreviousPage;
Expand Down
14 changes: 6 additions & 8 deletions src/components/organisms/Settings/Project/Dataset/hooks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import { useApolloClient } from "@apollo/client";
import { useCallback } from "react";

import { Item } from "@reearth/components/molecules/Settings/Project/Dataset/DatasetList";
import {
DatasetsListQuery,
useGetProjectSceneQuery,
useImportDatasetMutation,
useRemoveDatasetMutation,
Expand All @@ -11,10 +11,6 @@ import {
import { useT } from "@reearth/i18n";
import { useTeam, useProject, useNotification } from "@reearth/state";

type Nodes = NonNullable<DatasetsListQuery["datasetSchemas"]["nodes"]>;

type DatasetSchemas = NonNullable<Nodes[number]>[];

const datasetPerPage = 20;

export default (projectId: string) => {
Expand All @@ -38,9 +34,11 @@ export default (projectId: string) => {
notifyOnNetworkStatusChange: true,
});

const nodes = data?.datasetSchemas.edges.map(e => e.node) ?? [];

const datasetSchemas = nodes.filter(Boolean) as DatasetSchemas;
const datasetSchemas = data?.datasetSchemas.edges
.map<Item | undefined>(item =>
item.node ? { id: item.node.id, name: item.node.name } : undefined,
)
.filter((item): item is Item => !!item);
Comment on lines +37 to +41
Copy link
Member

Choose a reason for hiding this comment

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

Same as above: destructure the node and then you don't need to assign the values to keys. And again, not sure if the filter is necessary? Let me know


const hasMoreDataSets =
data?.datasetSchemas?.pageInfo.hasNextPage || data?.datasetSchemas?.pageInfo.hasPreviousPage;
Expand Down
Loading