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

Fix: Implement skeleton view in workspace member page #43893

Merged
merged 13 commits into from
Jun 25, 2024
Merged

Fix: Implement skeleton view in workspace member page #43893

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
e80539f
Fix: Implement skeleton view in workspace member page
truph01 Jun 18, 2024
135663a
Fix: Remove two last lines
truph01 Jun 18, 2024
c97a944
Fix: Type
truph01 Jun 18, 2024
c0f0d07
Fix: Merge main
truph01 Jun 19, 2024
aea1528
Fix: Lint
truph01 Jun 19, 2024
d38cf0b
Fix: Lint
truph01 Jun 19, 2024
a1b38f8
Fix: Lint
truph01 Jun 19, 2024
02d824f
Fix: Conflict
truph01 Jun 25, 2024
ee1ec27
Fix: Remove inline styles
truph01 Jun 25, 2024
2b60ac5
Fix: Remove UserListSkeletonView
truph01 Jun 25, 2024
08db8dc
Fix: Comment
truph01 Jun 25, 2024
8913b18
Fix: Using padding left 16px
truph01 Jun 25, 2024
1802c67
Fix: Apply x position 68 to two stacked lines
truph01 Jun 25, 2024
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
5 changes: 4 additions & 1 deletion src/components/SelectionList/BaseSelectionList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ function BaseSelectionList<TItem extends ListItem>(
{
sections,
ListItem,
SkeletonView,
canSelectMultiple = false,
onSelectRow,
shouldDebounceRowSelect = false,
Expand Down Expand Up @@ -628,6 +629,8 @@ function BaseSelectionList<TItem extends ListItem>(
},
);

const getSkeletonView = useCallback(() => (SkeletonView ? <SkeletonView shouldAnimate /> : <OptionsListSkeletonView shouldAnimate />), [SkeletonView]);

return (
<SafeAreaConsumer>
{({safeAreaPaddingBottomStyle}) => (
Expand Down Expand Up @@ -680,7 +683,7 @@ function BaseSelectionList<TItem extends ListItem>(
)}
{!!headerContent && headerContent}
{flattenedSections.allOptions.length === 0 && showLoadingPlaceholder ? (
<OptionsListSkeletonView shouldAnimate />
getSkeletonView()
) : (
<>
{!listHeaderContent && header()}
Expand Down
7 changes: 7 additions & 0 deletions src/components/SelectionList/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,13 +260,19 @@ type SectionWithIndexOffset<TItem extends ListItem> = Section<TItem> & {
indexOffset?: number;
};

type SkeletonViewProps = {
shouldAnimate: boolean;
};

type BaseSelectionListProps<TItem extends ListItem> = Partial<ChildrenProps> & {
/** Sections for the section list */
sections: Array<SectionListDataType<TItem>> | typeof CONST.EMPTY_ARRAY;

/** Default renderer for every item in the list */
ListItem: ValidListItem;

SkeletonView?: React.FC<SkeletonViewProps>;

/** Whether this is a multi-select list */
canSelectMultiple?: boolean;

Expand Down Expand Up @@ -485,6 +491,7 @@ export type {
ReportListItemProps,
ReportListItemType,
Section,
SkeletonViewProps,
SectionListDataType,
SectionWithIndexOffset,
SelectionListHandle,
Expand Down
28 changes: 16 additions & 12 deletions src/components/Skeletons/ItemListSkeletonView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import React, {useMemo, useState} from 'react';
import {View} from 'react-native';
import type {StyleProp, ViewStyle} from 'react-native';
import SkeletonViewContentLoader from '@components/SkeletonViewContentLoader';
import useTheme from '@hooks/useTheme';
import useThemeStyles from '@hooks/useThemeStyles';
Expand All @@ -9,9 +10,10 @@ type ListItemSkeletonProps = {
shouldAnimate?: boolean;
renderSkeletonItem: (args: {itemIndex: number}) => React.ReactNode;
fixedNumItems?: number;
itemContainerStyle?: StyleProp<ViewStyle>;
};

function ItemListSkeletonView({shouldAnimate = true, renderSkeletonItem, fixedNumItems}: ListItemSkeletonProps) {
function ItemListSkeletonView({shouldAnimate = true, renderSkeletonItem, fixedNumItems, itemContainerStyle}: ListItemSkeletonProps) {
const theme = useTheme();
const themeStyles = useThemeStyles();

Expand All @@ -20,20 +22,22 @@ function ItemListSkeletonView({shouldAnimate = true, renderSkeletonItem, fixedNu
const items = [];
for (let i = 0; i < numItems; i++) {
items.push(
<SkeletonViewContentLoader
key={`skeletonViewItems${i}`}
animate={shouldAnimate}
height={CONST.LHN_SKELETON_VIEW_ITEM_HEIGHT}
backgroundColor={theme.skeletonLHNIn}
foregroundColor={theme.skeletonLHNOut}
style={themeStyles.mr5}
>
{renderSkeletonItem({itemIndex: i})}
</SkeletonViewContentLoader>,
<View style={itemContainerStyle}>
<SkeletonViewContentLoader
key={`skeletonViewItems${i}`}
animate={shouldAnimate}
height={CONST.LHN_SKELETON_VIEW_ITEM_HEIGHT}
backgroundColor={theme.skeletonLHNIn}
foregroundColor={theme.skeletonLHNOut}
style={themeStyles.mr5}
>
{renderSkeletonItem({itemIndex: i})}
</SkeletonViewContentLoader>
</View>,
);
}
return items;
}, [numItems, shouldAnimate, theme, themeStyles, renderSkeletonItem]);
}, [numItems, shouldAnimate, theme, themeStyles, renderSkeletonItem, itemContainerStyle]);

return (
<View
Expand Down
41 changes: 41 additions & 0 deletions src/components/UserListSkeletonView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import React from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is extremely similar to OptionsListSkeletonView, I'm wondering if it would be better to just pass a prop like isTableList to OptionsListSkeletonView, which would keep things more DRY. Any thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jjcoffee Sorry, you mean that we should remove the UserListSkeletonView component, then handle it in OptionListSkeletonView by passing prop isUserList, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I think that's tidier, unless you have any other thoughts! I think isTableList as a prop name fits better here as we're only styling the list as a table, rather than changing the layout from the base layout to fit something specific for a user list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jjcoffee In the main branch, we already have TableListItemSkeleton, ItemListSkeletonView, and OptionListSkeletonView (which is based on ItemListSkeletonView). In this PR, we just added a new skeleton component, UserListSkeletonView. To maintain consistency with other skeleton components, we should keep UserListSkeletonView as is. If we plan to let OptionListSkeletonView handle UserListSkeletonView by passing isUserList, we should apply the same approach to the other skeleton components like TableListItemSkeleton and ItemListSkeletonView.

Copy link
Contributor

@jjcoffee jjcoffee Jun 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

My point is that UserListSkeletonView is almost identical to OptionListSkeletonView, so I'm not sure how useful it is to split it out into a new component since it doesn't apply any layout changes, just a few style tweaks (as far as I can tell). OptionListSkeletonView and TableListItemSkeleton both use ItemListSkeletonView, so I'm not sure your point applies here. Unless I'm missing something!

Copy link
Contributor Author

@truph01 truph01 Jun 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

OptionListSkeletonView uses TableListItemSkeleton which uses ItemListSkeletonView

Sorry, why did you say that OptionListSkeletonView uses TableListItemSkeleton? Is there any mistake here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Haha oops sorry, I meant that they both use ItemListSkeletonView. We are displaying an OptionList here, so I don't know if it makes sense to add a new component as not all user lists should be displayed as tables. I think we are fundamentally saying "here's an option list, but we want it to look like a table". TableListItemSkeleton uses a completely different layout (not just styling), that's why it needs to be split into a separate component.

import {Circle, Rect} from 'react-native-svg';
import useTheme from '@hooks/useTheme';
import type {SkeletonViewProps} from './SelectionList/types';
import ItemListSkeletonView from './Skeletons/ItemListSkeletonView';

function UserListSkeletonView({shouldAnimate = true}: SkeletonViewProps) {
const theme = useTheme();

return (
<ItemListSkeletonView
shouldAnimate={shouldAnimate}
itemContainerStyle={{backgroundColor: theme.highlightBG, marginBottom: 12, marginHorizontal: 20, borderRadius: 8}}
renderSkeletonItem={() => (
<>
<Circle
cx="40"
cy="32"
r="20"
/>
<Rect
x="72"
y="18"
width="20%"
height="8"
/>
<Rect
x="72"
y="38"
width="10%"
height="8"
/>
</>
)}
/>
);
}

UserListSkeletonView.displayName = 'UserListSkeletonView';

export default UserListSkeletonView;
2 changes: 2 additions & 0 deletions src/pages/workspace/WorkspaceMembersPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import SelectionList from '@components/SelectionList';
import TableListItem from '@components/SelectionList/TableListItem';
import type {ListItem, SelectionListHandle} from '@components/SelectionList/types';
import Text from '@components/Text';
import UserListSkeletonView from '@components/UserListSkeletonView';
import type {WithCurrentUserPersonalDetailsProps} from '@components/withCurrentUserPersonalDetails';
import withCurrentUserPersonalDetails from '@components/withCurrentUserPersonalDetails';
import useLocalize from '@hooks/useLocalize';
Expand Down Expand Up @@ -567,6 +568,7 @@ function WorkspaceMembersPage({personalDetails, invitedEmailsToAccountIDsDraft,
canSelectMultiple={isPolicyAdmin}
sections={[{data, isDisabled: false}]}
ListItem={TableListItem}
SkeletonView={UserListSkeletonView}
disableKeyboardShortcuts={removeMembersConfirmModalVisible}
headerMessage={getHeaderMessage()}
headerContent={!isSmallScreenWidth && getHeaderContent()}
Expand Down
Loading