-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
feat(web): the table skeleton loading #3628
Conversation
NV-2425 Implement the Integrations List Page
What?Implement the new Integrations List page according to the new designs:
The show sidebar functionality on The sorting and filtering functionality is out of scope for this cycle, also the conditions column is out of scope. Do not remove the old page because we will show/hide the old/new page with the feature flag mechanism. Definition of Done
Design specList Integration store / List view - Integration store (Figma) Starting state Integration store / List view - Integration store (Figma) Empty state Integration store / List view - Integration store (Figma) Skeleton Integration store / List view - Integration store (Figma) Add a provider button Add a provider/workflow button - Integration store (Figma) Provider logos TBD - waiting for Pixel Point. Will be posted here |
@@ -64,7 +64,7 @@ | |||
"@types/node": "^12.0.0", | |||
"@types/react": "^17.0.0", | |||
"@types/react-dom": "^17.0.0", | |||
"@types/react-table": "^7.7.9", | |||
"@types/react-table": "^7.7.12", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated slightly react-table
and types package, that have more precise types
const DefaultCellComponent = ({ value }: IExtendedCellProps) => { | ||
return value ?? ''; | ||
}; | ||
|
||
export const DefaultCell = withCellLoading(DefaultCellComponent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a default table cell component that will show the loading skeleton then the table is in the loading state. This component will be used if we do not override the column Cell
property.
const Template: ComponentStory<typeof Table> = ({ ...args }) => ( | ||
<Table columns={columns as any} data={data} {...args} /> | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed some ts issues in the storybook
'& tbody tr[data-disabled="true"]:hover': { | ||
cursor: 'default', | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when the table row is in the disabled state, remove the cursor pointer
export type IExtendedCellProps<T extends object = {}> = CellProps<T> & { isLoading: boolean }; | ||
|
||
export interface ITableProps { | ||
columns?: ColumnWithStrictAccessor<Data>[]; | ||
data?: Data[]; | ||
export type IExtendedColumn<T extends object = {}> = Column<T> & { | ||
Cell?: (props: IExtendedCellProps<T>) => React.ReactNode; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the column cell props types. Previously we were only passing the original
object and the types were not working when we've been defining the table columns
array. Passing only the original
object was limiting us, so I've changed it to what the actual library provides + plus additional props like isLoading
.
{ | ||
accessor: 'name', | ||
Header: 'Name', | ||
Cell: ({ name }: any) => ( | ||
<Tooltip label={name}> | ||
Cell: withCellLoading(({ row: { original } }) => ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we do override the Cell
and want to have a skeleton we would have to wrap the component with the HOC for loading, otherwise we would have to handle the loading state manually, in some cases, it might be useful, for ex. if you want to show the different loading skeletons.
}, | ||
{ | ||
accessor: 'createdAt', | ||
Header: 'Created At', | ||
Cell: ({ createdAt }: any) => format(new Date(createdAt), 'dd/MM/yyyy HH:mm'), | ||
Cell: withCellLoading(({ row: { original } }) => format(new Date(original.createdAt ?? ''), 'dd/MM/yyyy HH:mm')), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now the types are fixed and we don't need to use any
anymore 🎉
@@ -154,40 +157,39 @@ export function LayoutsListPage({ handleLayoutAnalytics }: LayoutsListPageProps) | |||
<LayoutEditor goBack={goBack} /> | |||
</When> | |||
<When truthy={activeScreen === ActivePageEnum.LAYOUTS_LIST}> | |||
<LoadingOverlay visible={isLoading || isLoadingDelete}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed the loading overlay as the table will show the loading state
@@ -49,79 +48,85 @@ export const ChangesTable = ({ | |||
}); | |||
}, [error]); | |||
|
|||
const columns: ColumnWithStrictAccessor<Data>[] = [ | |||
const columns: IExtendedColumn<any>[] = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wasn't able to find the type
if (isLoading) { | ||
return null; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved this statement to outside
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should show a spinner or something if it takes long time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, we can have a loading overlay that we do show everywhere currently :)
5b527c5
to
3227b83
Compare
@@ -18,7 +18,7 @@ import LoginPage from './pages/auth/LoginPage'; | |||
import SignUpPage from './pages/auth/SignUpPage'; | |||
import HomePage from './pages/HomePage'; | |||
import TemplateEditorPage from './pages/templates/editor/TemplateEditorPage'; | |||
import NotificationList from './pages/templates/TemplatesListPage'; | |||
import WorkflowListPage from './pages/templates/WorkflowListPage'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and renamed this one :P
@@ -25,6 +25,14 @@ interface IChannelDefinition { | |||
type: NodeTypeEnum; | |||
} | |||
|
|||
export const CHANNEL_TYPE_TO_STRING: Record<ChannelTypeEnum, string> = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it possible to use this one inside of WorkflowNode
as well so we have this mapping only once?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, just need to lower-case it in the WorkflowNode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
…ggestions from the pr #3628
What change does this PR introduce?
This is a preparation for the new Integrations List Page.
The changes:
react-table
minor version which has better typesTable
component to allow having a skeleton loading per cell (according to the latest designs)Cell
prop is not overridden it will use the default cell loading componentcolumn
prop typestitle
prop of thePageContainer
Why was this change needed?
Other information (Screenshots)
Screen.Recording.2023-06-20.at.17.24.04.mov