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

Added Skeleton for every pages #530

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

Conversation

abhayymishraa
Copy link
Collaborator

@abhayymishraa abhayymishraa commented Jan 17, 2025

Resolves #420

Add the PR description here.

  • Added dark/light mode support in skeleton components
  • Fixed testcases for the Skeleton
  • Replaced Loaders with the Skeleton

Preview

Screencast.from.17-01-25.01.12.09.PM.IST.webm

Copy link
Collaborator

@arkid15r arkid15r left a comment

Choose a reason for hiding this comment

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

Works for me, could you resolve these:

frontend/src/components/skeletons/ChapterSkeleton.tsx Outdated Show resolved Hide resolved
frontend/src/components/skeletons/CommunitySkeleton.tsx Outdated Show resolved Hide resolved
frontend/src/components/skeletons/ContributeSkeleton.tsx Outdated Show resolved Hide resolved
frontend/src/components/skeletons/ProjectSkeleton.tsx Outdated Show resolved Hide resolved
frontend/src/components/skeletons/UserDetailSkeleton.tsx Outdated Show resolved Hide resolved
frontend/src/components/ui/card.tsx Outdated Show resolved Hide resolved
frontend/src/components/skeletons/ProjectSkeleton.tsx Outdated Show resolved Hide resolved
frontend/src/components/SearchPageLayout.tsx Outdated Show resolved Hide resolved
frontend/src/components/SearchPageLayout.tsx Outdated Show resolved Hide resolved
frontend/src/components/SearchPageLayout.tsx Outdated Show resolved Hide resolved
@arkid15r arkid15r requested review from aramattamara, Dishant1804, harsh3dev and yashgoyal0110 and removed request for arkid15r January 17, 2025 19:53
Copy link
Collaborator

@Dishant1804 Dishant1804 left a comment

Choose a reason for hiding this comment

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

Looks good, covers all the loaders which are present

@yashgoyal0110
Copy link
Collaborator

@abhayymishraa concept is good but what if there's a change in card layout or designing or some change in page design occurs in near future due to any requirement, then will you keep changing skeleton layout again and again?

@arkid15r what's your take?

@arkid15r
Copy link
Collaborator

@abhayymishraa concept is good but what if there's a change in card layout or designing or some change in page design occurs in near future due to any requirement, then will you keep changing skeleton layout again and again?

@arkid15r what's your take?

I don't know if there is a way to avoid it, e.g. autogenerate the skeletons? I think keeping the skeletons structure simple (not 1:1 copy of a real component) may allow us to avoid additional work?

@yashgoyal0110
Copy link
Collaborator

@abhayymishraa concept is good but what if there's a change in card layout or designing or some change in page design occurs in near future due to any requirement, then will you keep changing skeleton layout again and again?
@arkid15r what's your take?

I don't know if there is a way to avoid it, e.g. autogenerate the skeletons? I think keeping the skeletons structure simple (not 1:1 copy of a real component) may allow us to avoid additional work?

yes i also think we should have generic template to reduce frequent updates with ui changes

@Dishant1804
Copy link
Collaborator

Dishant1804 commented Jan 17, 2025

@abhayymishraa concept is good but what if there's a change in card layout or designing or some change in page design occurs in near future due to any requirement, then will you keep changing skeleton layout again and again?
@arkid15r what's your take?

I don't know if there is a way to avoid it, e.g. autogenerate the skeletons? I think keeping the skeletons structure simple (not 1:1 copy of a real component) may allow us to avoid additional work?

There is a library that i've used in past it auto generates the skeleton for the components, it is called react-loading-skeleton, this will automatically adapt to the component

@yashgoyal0110
Copy link
Collaborator

@abhayymishraa concept is good but what if there's a change in card layout or designing or some change in page design occurs in near future due to any requirement, then will you keep changing skeleton layout again and again?
@arkid15r what's your take?

I don't know if there is a way to avoid it, e.g. autogenerate the skeletons? I think keeping the skeletons structure simple (not 1:1 copy of a real component) may allow us to avoid additional work?

There is a library that i've used in past it auto generates the skeleton for the components, it is called react-loading-skeleton, this will automatically adapt to the compoennt

@abhayymishraa can you please use same sort of library for future maintainability

@abhayymishraa
Copy link
Collaborator Author

I think @arkid15r and @yashgoyal0110 creating a skeleton is easy!! Like you just need to copy the classes and just need to put skeleton in the place of input or any button , so I think it will be easy to make skeleton for the components, also there is no autogenerated skeleton i think.

@abhayymishraa
Copy link
Collaborator Author

Making it generic wouldn't work cause we have a lot of different types of components and also there will be a lot of addition so what i think making a skeleton for every page will work as we only have to make the skeleton for the pages we have

@Dishant1804
Copy link
Collaborator

@abhayymishraa for a longer run we cannot just do that for every component it will resulting in creation of lot of files just for skeleton
@arkid15r your view? i am suggesting that react-loading-skeleton is good library to look forward to

@abhayymishraa
Copy link
Collaborator Author

@Dishant1804 i will look into it !!!!

@arkid15r
Copy link
Collaborator

@abhayymishraa for a longer run we cannot just do that for every component it will resulting in creation of lot of files just for skeleton @arkid15r your view? i am suggesting that react-loading-skeleton is good library to look forward to

It sounds like we could automate the skeletons generation? if that's the case it would definitely save time and be the preferred approach.

Copy link
Collaborator

@kasya kasya left a comment

Choose a reason for hiding this comment

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

@abhayymishraa hi! Is this still work in progress or is it ready for review? I was not sure after the discussion. Also, there are conflicts that need to be resolved before any review 👍🏼

@abhayymishraa
Copy link
Collaborator Author

Heyy what from my point of view what i saw in the react-loading-skeleton that we still need to manually add the skeleton component and configure to look like it , so we can't say it that it generates skelton so i think like maximum we have now 4-5 pages and most of these are re-usable so we can use them also configure if we have a different ui, i think we should go with my approach for now as harsh suggested it cant be dont but we have to use conditional statements in all places!! i think we can go the approach what i thought of, we can go with harsh approach but in that case in future we have more component then we need to make it for all the added component so i think making it for the pages can be a good approach that i took !!
@harsh3dev @arkid15r

@arkid15r
Copy link
Collaborator

Heyy what from my point of view what i saw in the react-loading-skeleton that we still need to manually add the skeleton component and configure to look like it , so we can't say it that it generates skelton so i think like maximum we have now 4-5 pages and most of these are re-usable so we can use them also configure if we have a different ui, i think we should go with my approach for now as harsh suggested it cant be dont but we have to use conditional statements in all places!! i think we can go the approach what i thought of, we can go with harsh approach but in that case in future we have more component then we need to make it for all the added component so i think making it for the pages can be a good approach that i took !! @harsh3dev @arkid15r

@Dishant1804 could you share more details about your experience of using react-loading-skeleton?

@Dishant1804
Copy link
Collaborator

Dishant1804 commented Jan 20, 2025

Heyy what from my point of view what i saw in the react-loading-skeleton that we still need to manually add the skeleton component and configure to look like it , so we can't say it that it generates skelton so i think like maximum we have now 4-5 pages and most of these are re-usable so we can use them also configure if we have a different ui, i think we should go with my approach for now as harsh suggested it cant be dont but we have to use conditional statements in all places!! i think we can go the approach what i thought of, we can go with harsh approach but in that case in future we have more component then we need to make it for all the added component so i think making it for the pages can be a good approach that i took !! @harsh3dev @arkid15r

@Dishant1804 could you share more details about your experience of using react-loading-skeleton?

I used it and configured it a bit, the main moto why i used it was to get the building structure of the component ready without worrying much about the appearance, it does not completely auto generate as we expect here(but it gives/generates the structure we need), we have to set height and width according to our component, very minimal configuration was required when i used it

@abhayymishraa
Copy link
Collaborator Author

@Dishant1804 @arkid15r but we have some pages that require more than just simple configuration like there is project details page we can't only just use a single skeleton and configure it , if used it definitely doesn't look good as it doesn't have that much details !! And if we still need to configure it manually i think its breaking our main motive of using it, that is for autogeneration. Cause in that page we have different components so we need to manually change settings !! I think we thought for autogeneration .. that doesn't provided by this library.... What i think is making a skeleton per page will fullfill tha use case here

@abhayymishraa abhayymishraa requested a review from kasya January 20, 2025 17:11
@arkid15r
Copy link
Collaborator

I checked the react-loading-skeleton package and it seems using pretty convenient approach for keeping skeletons tied to the code. My understanding it that using this library we would need update 3 files only:

  • components/Card
  • pages/Users
  • pages/UserDetails

Please correct me if I'm wrong @Dishant1804

This approach is more structured and minimizes code repetition and maintenance efforts.

@Dishant1804
Copy link
Collaborator

Dishant1804 commented Jan 20, 2025

I checked the react-loading-skeleton package and it seems using pretty convenient approach for keeping skeletons tied to the code. My understanding it that using this library we would need update 3 files only:

  • components/Card
  • pages/Users
  • pages/UserDetails

Please correct me if I'm wrong @Dishant1804

This approach is more structured and minimizes code repetition and maintenance efforts.

@arkid15r you are absolutely correct, the main advantage of using it is the uniformity amongst the structure of skeleton it provides, it also reduces overhead of configuring everything

@arkid15r
Copy link
Collaborator

@Dishant1804 @arkid15r but we have some pages that require more than just simple configuration like there is project details page we can't only just use a single skeleton and configure it , if used it definitely doesn't look good as it doesn't have that much details !! And if we still need to configure it manually i think its breaking our main motive of using it, that is for autogeneration. Cause in that page we have different components so we need to manually change settings !! I think we thought for autogeneration .. that doesn't provided by this library.... What i think is making a skeleton per page will fullfill tha use case here

Let's focus on keeping it simple and maintainable w/ as low effort as possible. Having to make changes for the skeletons every time we change related components is a no-go. I suggest updating the Card code with generic skeleton structure that will cover all card-based entities (projects/chapters/committees) and also take care of user-related pages.

Let me know your thoughts about this approach @abhayymishraa

@abhayymishraa
Copy link
Collaborator Author

@arkid15r @Dishant1804 so should i wait for the component to be replaced by the chakra ui or should i install chakra ui dependecy and make this skeleton !!!!? i this talk should wait until the components were replaced by chakra ui !!!

@yashgoyal0110
Copy link
Collaborator

@abhayymishraa I think this shimmer effect has nothing to do with existing code files, as you are just changing loader, so i think you can external library.
@arkid15r what's your take?

@Dishant1804
Copy link
Collaborator

@arkid15r @Dishant1804 so should i wait for the component to be replaced by the chakra ui or should i install chakra ui dependecy and make this skeleton !!!!? i this talk should wait until the components were replaced by chakra ui !!!

You can implement the skeleton loading using chakra ui as we were already finding a long term solution for skeleton structure right, so i think as chakra ui is approved we need to maintain the uniformity and stick to one thing as long as possible. So in my opinion you can move forward with chakraUI's skeleton if @arkid15r agrees to this approach

@abhayymishraa
Copy link
Collaborator Author

@Dishant1804 So can i move forward to setup chakra ui for repo and work on skeleton???

@Dishant1804
Copy link
Collaborator

@Dishant1804 So can i move forward to setup chakra ui for repo and work on skeleton???

We need to take @arkid15r opinion to move forward with it let him approve as he is the maintainer of repo

@arkid15r
Copy link
Collaborator

@Dishant1804 So can i move forward to setup chakra ui for repo and work on skeleton???

We need to take @arkid15r opinion to move forward with it let him approve as he is the maintainer of repo

We're good to go with Chakra UI as per the recent discussion and the poll. I'm not sure about the wider refactoring vs this particular issue implementation situation though. @Dishant1804 I'll let you decide on how to handle this:

  • wait until we have the initial setup (or more) implemented and finish this later
  • go ahead with completing this issue as it might not need a lot of code changes (I don't know)

@Dishant1804
Copy link
Collaborator

Dishant1804 commented Jan 21, 2025

@Dishant1804 So can i move forward to setup chakra ui for repo and work on skeleton???

We need to take @arkid15r opinion to move forward with it let him approve as he is the maintainer of repo

We're good to go with Chakra UI as per the recent discussion and the poll. I'm not sure about the wider refactoring vs this particular issue implementation situation though. @Dishant1804 I'll let you decide on how to handle this:

  • wait until we have the initial setup (or more) implemented and finish this later
  • go ahead with completing this issue as it might not need a lot of code changes (I don't know)

@abhayymishraa You can go ahead with the implementation of chakraUI skeleton here, as it won't cause much hindrance in the code and wont require much refactoring after the implementation of this issue, try to make it more reusable and abstract, so that it can be used for future needs

@abhayymishraa
Copy link
Collaborator Author

abhayymishraa commented Jan 21, 2025

@Dishant1804 @arkid15r Sure, I'll take this up and try to implement it. By the way, I have a quick question. Shadcn provides a components.json file that specifies where everything should be installed, but from what I’ve researched, Chakra UI doesn’t seem to have a similar feature.?? SO how can we manage our code structure

@Dishant1804
Copy link
Collaborator

Dishant1804 commented Jan 22, 2025

@Dishant1804 @arkid15r Sure, I'll take this up and try to implement it. By the way, I have a quick question. Shadcn provides a components.json file that specifies where everything should be installed, but from what I’ve researched, Chakra UI doesn’t seem to have a similar feature.?? SO how can we manage our code structure

@abhayymishraa some components doesn't need to be installed (e.g. Card), however for some components like skeleton as reference for this issue needs to be installed via executable command, the component will be default installed in components/ui folder, i dont think so we need to configure that. So just install the skeleton with given command and just use it

@abhayymishraa
Copy link
Collaborator Author

i'm on it

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.

Replace Current Loader with the Skeleton Component
6 participants