refactor: refactor the infinite scroll wrapper and queries #267

nina992 · 2022-07-08T09:01:30Z

Overview

The changes:
1- Refactoring the queries of graphql and remove nodes
2- Modify all the queries on hooks
3- Create (infinite scroll) wrapper atom and reuse it wherever the need for infinite scroll

What I've done

The pages affected by this changes :
1- Dashboard page
2- setting pages :
1- Project list page
2- Assets page
3- Datasets page
4- setting page for each page
5- accessing pages from reearth-editor

What I haven't done

How I tested

Screenshot

Which point I want you to review particularly

Memo

netlify · 2022-07-08T09:02:47Z

✅ Deploy Preview for reearth-web ready!

Name	Link
🔨 Latest commit	`a71bb38`
🔍 Latest deploy log	https://app.netlify.com/sites/reearth-web/deploys/62c7f26b0bbd7e000837fac8
😎 Deploy Preview	https://deploy-preview-267--reearth-web.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

codecov · 2022-07-08T09:06:19Z

Codecov Report

Merging #267 (a71bb38) into main (eb41daf) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #267   +/-   ##
=======================================
  Coverage   52.19%   52.19%           
=======================================
  Files          56       56           
  Lines        1205     1205           
  Branches      193      193           
=======================================
  Hits          629      629           
  Misses        503      503           
  Partials       73       73

Impacted Files	Coverage Δ
src/gql/graphql-client-api.tsx	`35.78% <ø> (ø)`

KaWaite · 2022-07-13T01:04:00Z

src/components/atoms/InfiniteScroll/hooks.ts

+    if (ref.current && ref.current?.scrollHeight <= document.documentElement.clientHeight) {
+      onLoadMore?.();
+    }


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.

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?

KaWaite · 2022-07-13T01:20:34Z

src/components/organisms/Common/AssetContainer/hooks.ts

+  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);


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?

KaWaite · 2022-07-13T01:35:27Z

src/components/organisms/Dashboard/hooks.ts

  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]);


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

KaWaite · 2022-07-13T01:36:29Z

src/components/organisms/Settings/Project/Dataset/hooks.ts

+  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);


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

KaWaite · 2022-07-13T01:37:31Z

src/components/organisms/Settings/Project/hooks.ts

  const project = useMemo(
    () =>
-      rawProject?.id
+      rawProject?.node?.id


Destructure node

KaWaite · 2022-07-13T01:37:51Z

src/components/organisms/Settings/ProjectList/hooks.ts

    .map<Project | undefined>(project =>
-      project
+      project.node


Destructure node

KaWaite · 2022-07-13T01:40:58Z

src/components/atoms/InfiniteScroll/index.tsx

+  const wrapperRef = useRef<HTMLDivElement>(null);
+  const { handleAutoFillPage, handleScrollToBottom } = useHooks(onGetMoreItems);
+
+  useEffect(() => {
+    if (wrapperRef.current && !loading && hasMoreItems) handleAutoFillPage(wrapperRef);
+  }, [handleAutoFillPage, hasMoreItems, loading, onGetMoreItems]);


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).

KaWaite

Looks really good! A few clean up comments but really good refactor!

KaWaite · 2022-07-13T01:49:31Z

src/components/organisms/Settings/Project/Public/hooks.ts

@@ -42,26 +42,27 @@ export default ({ projectId }: Params) => {
  });

  const rawProject = useMemo(
-    () => data?.projects.nodes.find(p => p?.id === projectId),
+    () => data?.projects.edges.find(p => p.node?.id === projectId),


If you don't need rawProject's other fields (I forget what comes along with node...) you can destructure one step further to the node.
Then, below you can simplify like the other comments.

refactoring

a71bb38

nina992 marked this pull request as ready for review July 8, 2022 09:11

nina992 requested a review from KaWaite as a code owner July 8, 2022 09:11

KaWaite reviewed Jul 13, 2022

View reviewed changes

src/components/organisms/Settings/ProjectList/hooks.ts

.map<Project | undefined>(project =>

project

project.node

Copy link

Member

KaWaite Jul 13, 2022

Choose a reason for hiding this comment

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

Destructure node

KaWaite reviewed Jul 13, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: refactor the infinite scroll wrapper and queries #267

refactor: refactor the infinite scroll wrapper and queries #267

nina992 commented Jul 8, 2022

netlify bot commented Jul 8, 2022 •

edited

Loading

codecov bot commented Jul 8, 2022 •

edited

Loading

KaWaite Jul 13, 2022

airslice Jul 13, 2022 •

edited

Loading

KaWaite Jul 13, 2022

KaWaite Jul 13, 2022

KaWaite Jul 13, 2022

KaWaite Jul 13, 2022

KaWaite Jul 13, 2022

KaWaite Jul 13, 2022

KaWaite left a comment

KaWaite Jul 13, 2022

refactor: refactor the infinite scroll wrapper and queries #267

Are you sure you want to change the base?

refactor: refactor the infinite scroll wrapper and queries #267

Conversation

nina992 commented Jul 8, 2022

Overview

What I've done

What I haven't done

How I tested

Screenshot

Which point I want you to review particularly

Memo

netlify bot commented Jul 8, 2022 • edited Loading

✅ Deploy Preview for reearth-web ready!

codecov bot commented Jul 8, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

airslice Jul 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KaWaite left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netlify bot commented Jul 8, 2022 •

edited

Loading

codecov bot commented Jul 8, 2022 •

edited

Loading

airslice Jul 13, 2022 •

edited

Loading