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.

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

Add Skeleton loading for side panel #7394

Merged
merged 4 commits into from
Oct 4, 2024
Merged

Add Skeleton loading for side panel #7394

merged 4 commits into from
Oct 4, 2024

Conversation

gitstart-app[bot]
Copy link
Contributor

@gitstart-app gitstart-app bot commented Oct 2, 2024

This PR was created by GitStart to address the requirements from this ticket: TWNTY-7112.


Description

  • To test you can use await new Promise(r => setTimeout(r, 5000)); line 74 of `openCreateActivityDrawer.ts`

  • We added a recoil state to define the loading status

  • Design points to note:

    1 - We did not change the chip component styles because would be unrelated to the issue can you confirm if you still need this change?

    2- In Figma, the loading state shows the Chip rendering an initial name before showing the loaded name, currently, we are rendering the correct name while loading, the change that makes this possible is below

    if we set it as null, the initial name would appear, but also the previous data in the state would affect the UI, passing the activityObjectNameSingular data allows us to clear the previous data, and make the Chip instantly updated, let us know if this behavior is fine, or if you still want an initial name to be rendered while is loading.

    3 - Currently, the loading state of the tabs does not affect the selected tab (auto-defined by the component) should we change this logic for all Tabs used in the app, or make this behavior optional by using props?

Demo

https://www.loom.com/share/590df738a8ec41e6b64232bde237c01f?sid=7f8f4e40-ec82-4282-a43d-452a1cf27f4a

Refs

#7112

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This pull request implements skeleton loading for the side panel to improve user experience during long loading times. Key changes include:

  • Added new isNewViewableRecordLoadingState in isNewViewableRecordLoading.ts to manage loading status
  • Updated RightDrawerTopBar to display a disabled Chip component during loading
  • Introduced ShowPageSummaryCardSkeletonLoader component for skeleton loading of summary cards
  • Modified ShowPageRightContainer to use skeleton loaders for tabs during loading
  • Updated useOpenCreateActivityDrawer hook to handle loading states for activity creation
  • Adjusted ShowPageActivityContainer to render empty content while loading

8 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings

Comment on lines +39 to 41
) : (
<></>
);
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Consider rendering a skeleton loader instead of an empty fragment for better UX during loading

Comment on lines +23 to +24
baseColor={theme.background.tertiary}
highlightColor={theme.background.transparent.lighter}
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Consider using theme.background.secondary for baseColor to match the desired behavior mentioned in the PR description

@gitstart-twenty
Copy link
Contributor

Hello @ijreilly and @Bonapara
I've left some questions about this PR on the description, could you take a look?

@Bonapara
Copy link
Member

Bonapara commented Oct 2, 2024

Hi @gitstart-twenty,

For 1. we should stick to the current design without chip indeed:

CleanShot 2024-10-02 at 17 02 59

For 2. What do you mean by initial name?

Not sure to understand 3.

Maybe @ijreilly will have some more input!

highlightColor={theme.background.transparent.lighter}
>
<StyledContainer>
<Skeleton height={40} width={40} />
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we could extract all these hardcoded values used in Sketelon components, so that we have a limited height diversity of skeleton components.

Like height_xs, height_s, ..., width_s...

@gitstart-twenty
Copy link
Contributor

Hello @Bonapara
about topic 2, in the Chip, while the data is loading, we can have a kind of placeholder name

so when the user will create a Note:

  • He will see the chip with Company, with disabled styles
  • after loading, he would see the Chip with Note, with regular styles
    image
    This was in the design previously, but it seems you already updated it, so I think we don't need to worry about it

about topic 3
in this image, you can see that the first tab is highlighted even if the entire component has the loading state as true
this happens because of the current logic of the component
image
in the design, all tabs should have the loading styles when the entire component has the loading state as true.
The question is, do you want to make this loading behavior for this component regardless of where it is used, or It's just required on this page?
depending on the answer we will have different solutions to implement

@gitstart-twenty
Copy link
Contributor

Hello @ijreilly

is it not problematic not to do this anymore ? what was this used for?

Yes, this change should be reverted, It's not affecting the loading but we also are not sure how this change could affect other parts, we will push the change once we get confirmations about the other topics

I think we could extract all these hardcoded values used in Sketelon components, so that we have a limited height diversity of skeleton components.

Could you give more details about this implementation?

@Bonapara
Copy link
Member

Bonapara commented Oct 2, 2024

@gitstart-twenty it was a design mistake indeed, we don't want to display company in the loading state if we're loading a note. So we can either display "note" in a disable state or a skeleton loader.

For 3) make sense to have this behavior everywhere but only when the page is loading and not a specific tab. Do you see what I mean? We don't want to deactivate all the tabs each time one is loading.

@gitstart-twenty
Copy link
Contributor

gitstart-twenty commented Oct 2, 2024

@Bonapara

Currently, the loading state is not for the tabs individually, their disabling style is implemented individually only if the single tab receives disabled = true.

The current loading state tries to apply the disabled styles to all items. This states refers to loading the entire list, and not each tab, but it will be overwritten if a single tab has disabled as false.

What do you think about the following:

  • If loading is true, all tabs will be disabled, even if individually it is not loading (disabled = false or disabled = undefined), and even if it was considered active (the problem I mentioned)
  • If loading is false, the disabled property of each tab will be considered (so we will not block the other tabs) and the active tab will be computed normally

@Bonapara
Copy link
Member

Bonapara commented Oct 3, 2024

I think we could stick to individual tab loading for now, following Charles's advice. In the example you gave:

about topic 3 in this image, you can see that the first tab is highlighted even if the entire component has the loading state as true this happens because of the current logic of the component image in the design, all tabs should have the loading styles when the entire component has the loading state as true. The question is, do you want to make this loading behavior for this component regardless of where it is used, or It's just required on this page? depending on the answer we will have different solutions to implement

Is the content under the "Note" tab already loaded? Or is the tab displayed as "highlighted without content?

@gitstart-twenty
Copy link
Contributor

@Bonapara
It's displayed as "highlighted" without content.

@Bonapara
Copy link
Member

Bonapara commented Oct 3, 2024

Ok I think we can keep it like this!

@ijreilly ijreilly merged commit 8afa504 into main Oct 4, 2024
6 of 10 checks passed
@ijreilly ijreilly deleted the TWNTY-7112 branch October 4, 2024 09:41
Copy link

github-actions bot commented Oct 4, 2024

Fails
🚫

node failed.

Log

�[31mError: �[39m SyntaxError: Unexpected end of JSON input
    at JSON.parse (<anonymous>)
�[90m    at parseJSONFromBytes (node:internal/deps/undici/undici:5584:19)�[39m
�[90m    at successSteps (node:internal/deps/undici/undici:5555:27)�[39m
�[90m    at fullyReadBody (node:internal/deps/undici/undici:1665:9)�[39m
�[90m    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)�[39m
�[90m    at async specConsumeBody (node:internal/deps/undici/undici:5564:7)�[39m
danger-results://tmp/danger-results-346cdd86.json

Generated by 🚫 dangerJS against fb78a68

harshit078 pushed a commit to harshit078/twenty that referenced this pull request Oct 14, 2024
This PR was created by [GitStart](https://gitstart.com/) to address the
requirements from this ticket:
[TWNTY-7112](https://clients.gitstart.com/twenty/5449/tickets/TWNTY-7112).

 --- 


### Description

- To test you can use `await new Promise(r => setTimeout(r, 5000));`
line 74 of \`openCreateActivityDrawer.ts\`
- We added a recoil state to define the loading status
- Design points to note:

1 - We did not change the chip component styles because would be
unrelated to the issue can you confirm if you still need this change?


![](https://assets-service.gitstart.com/28455/c5999ef1-a7fc-4c53-b425-d0588092ba90.png)

2- In Figma, the loading state shows the Chip rendering an initial name
before showing the loaded name, currently, we are rendering the correct
name while loading, the change that makes this possible is below


![](https://assets-service.gitstart.com/28455/a0e14045-9a14-4d19-9570-62781fba1aa4.png)

if we set it as null, the initial name would appear, but also the
previous data in the state would affect the UI, passing the
`activityObjectNameSingular` data allows us to clear the previous data,
and make the Chip instantly updated, let us know if this behavior is
fine, or if you still want an initial name to be rendered while is
loading.

3 - Currently, the loading state of the tabs does not affect the
selected tab (auto-defined by the component) should we change this logic
for all Tabs used in the app, or make this behavior optional by using
props?


![](https://assets-service.gitstart.com/28455/223c2e9f-3f4b-4107-b40d-f98a95266d5d.png)

### Demo


<https://www.loom.com/share/590df738a8ec41e6b64232bde237c01f?sid=7f8f4e40-ec82-4282-a43d-452a1cf27f4a>

### Refs

twentyhq#7112

---------

Co-authored-by: gitstart-twenty <[email protected]>
Co-authored-by: gitstart-twenty <[email protected]>
Co-authored-by: Marie Stoppa <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants