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.

Sign up for GitHub

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

Jump to bottom

User & Metadata Loading #5347

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

User & Metadata Loading #5347

wants to merge 3 commits into from

Conversation

gitstart-twenty
Copy link
Contributor

@gitstart-twenty gitstart-twenty commented May 10, 2024 •
edited

Description

User & Metadata Loading

Refs

#4456

Demo

prefetch_loading.mp4

Fixes #4456

@gitstart-twenty @v1b3m @Thiagomn1
User & Metadata Loading
116a0c2 
Co-authored-by: v1b3m <vibenjamin6@gmail.com>
Co-authored-by: Thiago Nascimbeni <tnascimbeni@gmail.com>
@gitstart-twenty @v1b3m @Thiagomn1
Fix types
543e1b7 
Co-authored-by: v1b3m <vibenjamin6@gmail.com>
Co-authored-by: Thiago Nascimbeni <tnascimbeni@gmail.com>
@Bonapara
Copy link
Member

Thanks!

  1. Could we tweak the left menu to ensure the loader aligns perfectly with the menu items? There are some variations in the gap or items height.

  2. I think we shouldn't display the view bar during User & Metadata Loading since we don't know which page layout will appear afterward (Record page, /tasks...). Or do we? Is it definitely an "Object Index"? If so, can you add a skeleton for the view switcher while we wait for the view name?

CleanShot.2024-05-10.at.18.13.13-converted.mp4
  1. Can you add a skeleton for the top right CTAs like in Figma?

@gitstart-twenty
Copy link
Contributor Author

Thanks!

  1. Could we tweak the left menu to ensure the loader aligns perfectly with the menu items? There are some variations in the gap or items height.
  2. I think we shouldn't display the view bar during User & Metadata Loading since we don't know which page layout will appear afterward (Record page, /tasks...). Or do we? Is it definitely an "Object Index"? If so, can you add a skeleton for the view switcher while we wait for the view name?

CleanShot.2024-05-10.at.18.13.13-converted.mp4
3. Can you add a skeleton for the top right CTAs like in Figma?

Alright

charlesBochet
charlesBochet reviewed May 14, 2024
View reviewed changes
packages/twenty-front/src/App.tsx Outdated
@@ -58,13 +60,15 @@ export const App = () => {
const { pathname } = useLocation();
const pageTitle = getPageTitleFromPath(pathname);

const { loading: isPrefetchLoading } = useContext(PrefetchContext);
Copy link
Member

Choose a reason for hiding this comment

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

We should not react to isPrefetchLoading here as this will trigger re-render of the whole app.
It should be loaded in the lowest places that needs it so only this part is re-rendered

Copy link
Member

Choose a reason for hiding this comment

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

@gitstart-twenty see my latest comment before treating review feedbacks. I think we should not check the prefetch loading state but the metadata and currentUser queries loading states

packages/twenty-front/src/modules/navigation/components/AppNavigationDrawer.tsx Outdated
};

export const AppNavigationDrawer = ({
className,
loading,
Copy link
Member

Choose a reason for hiding this comment

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

we should provide a default = false here

packages/twenty-front/src/modules/navigation/components/MainNavigationDrawerItems.tsx Outdated
margin-left: 12px;
`;

const StyledSkeletonLoader = ({
Copy link
Member

Choose a reason for hiding this comment

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

this is not a Styled component (it's actually containing much more) so we should not prefix it with Styled and it should go in its own file NavigationDrawerItemsSkeletonLoader for example

packages/twenty-front/src/modules/navigation/components/MainNavigationDrawerItems.tsx Outdated
return (
<>
<StyledSkeletonLoader length={4} />
<StyledSkeletonLoader title length={2} />
Copy link
Member

Choose a reason for hiding this comment

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

title props is not clear. withTitle?

...ges/twenty-front/src/modules/object-record/record-index/components/RecordIndexPageHeader.tsx
@@ -39,9 +41,11 @@ export const RecordIndexPageHeader = ({
objectMetadataItem?.labelPlural ?? capitalize(objectNamePlural);

return (
<PageHeader title={pageHeaderTitle} Icon={Icon}>
<PageHeader title={pageHeaderTitle} Icon={Icon} loading={loading}>
Copy link
Member

Choose a reason for hiding this comment

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

we should react to the loading status here. It will only re-render the Header

packages/twenty-front/src/modules/prefetch/components/PrefetchDataProvider.tsx Outdated
@@ -2,11 +2,16 @@ import React from 'react';

import { PrefetchRunQueriesEffect } from '@/prefetch/components/PrefetchRunQueriesEffect';

export const PrefetchContext = React.createContext({
Copy link
Member

Choose a reason for hiding this comment

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

let's avoid this pattern, it will trigger re-renders. Instead, we should store this prefetch loading state in a recoil state and fetch it as low as possible.

packages/twenty-front/src/modules/prefetch/components/PrefetchRunQueriesEffect.tsx Outdated
operationSignatures: [
FIND_ALL_VIEWS_OPERATION_SIGNATURE,
FIND_ALL_FAVORITES_OPERATION_SIGNATURE,
],
skip: !currentUser,
});

useEffect(() => {
setLoading(loading);
Copy link
Member

Choose a reason for hiding this comment

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

let's set a recoil state direclty here

packages/twenty-front/src/modules/ui/layout/page/PageAddButton.tsx
ariaLabel="Add"
/>
);
const StyledSkeletonLoader = () => {
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 :)

...enty-front/src/modules/ui/navigation/navigation-drawer/components/NavigationDrawerHeader.tsx
@@ -42,20 +44,41 @@ const StyledNavigationDrawerCollapseButton = styled(
transition: opacity ${({ theme }) => theme.animation.duration.normal}s;
`;

const StyledSkeletonLoader = () => {
Copy link
Member

Choose a reason for hiding this comment

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

same :)

@charlesBochet
Copy link
Member

@gitstart-twenty I have left comments but then I took another look at the ticket and I think there is a miscommunication.
The Prefetch Queries are actually loading the following data: favorites and views. It seems that the initial ticket was about userLoading and metadataLoading.

PrefetchQueries loading should impact:

  • the favorites menu only
  • the view bar only

User and Metadata should impact the what you actually have done in this PR. It does not change the overall strategy, I would still introduce a isMetadataLoadingState and isCurrentUserLoadingState (already exists) and leverage it where you are leveraging the prefetch loading state in this PR

@charlesBochet charlesBochet self-assigned this May 14, 2024
@gitstart-twenty
Copy link
Contributor Author

gitstart-twenty commented May 14, 2024 •
edited

@gitstart-twenty I have left comments but then I took another look at the ticket and I think there is a miscommunication. The Prefetch Queries are actually loading the following data: favorites and views. It seems that the initial ticket was about userLoading and metadataLoading.

PrefetchQueries loading should impact:

  • the favorites menu only
  • the view bar only

User and Metadata should impact the what you actually have done in this PR. It does not change the overall strategy, I would still introduce a isMetadataLoadingState and isCurrentUserLoadingState (already exists) and leverage it where you are leveraging the prefetch loading state in this PR

Hey @charlesBochet,
Thanks for the comments. Looking into them!

@gitstart-twenty @v1b3m @Thiagomn1
Refactor according to review
6443467 
Co-authored-by: v1b3m <vibenjamin6@gmail.com>
Co-authored-by: Thiago Nascimbeni <tnascimbeni@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@charlesBochet charlesBochet charlesBochet left review comments

@Bonapara Bonapara Awaiting requested review from Bonapara

At least 1 approving review is required to merge this pull request.

Assignees

@charlesBochet charlesBochet

Labels
PR: awaiting review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

User & Metadata Loading
3 participants
@gitstart-twenty @Bonapara @charlesBochet