Liisa/enhancement/623 implement skeleton loaders#647
Conversation
Add working skeletonLoader with small issues
-Added same layout as in NewsCard for loader -Made them appear responsively
-Added documentation for News Skeletonloader
davylis
requested review from
Rutjake,
Skoivumaki,
leolabdev and
patinen
as code owners
Skoivumaki
left a comment
Skoivumaki
left a comment
There was a problem hiding this comment.
Great job on this! I had a hard time catching the loader in action because the Directus queries load almost instantly. I was originally picturing a skeleton loader using Tailwind's pulse, but your implementation gets the job done perfectly it seems. (it looks like it has the pulse effect or similar) https://tailwindcss.com/docs/animation#adding-a-pulse-animation
|
|
||
| const limit = 6; | ||
| //limit for initial load(no news data fetched yet) | ||
| const limit = 12; |
There was a problem hiding this comment.
Increasing limit was not part of issue. I think it wont cause issues with infinite scroll.
There was a problem hiding this comment.
Good point. I’ll revert the limit back to 6
There was a problem hiding this comment.
Good work for appending a existing skeleton loader file with new stuff. Ultimately we should have a skeleton loader that can take the existing component and render on it, for all cases.
There was a problem hiding this comment.
Since the news cards have a very specific layout compared to other components, I created a news skeleton structure here. I agree that a reusable skeleton loader would be a better option and I’ll definitely try that approach next time!
|
|
||
| const pageIsLoading = isNewsLoading || isMoreNewsLoading || !data || !moreNews; | ||
|
|
||
| const getLngCode = (lng: string) => { |
There was a problem hiding this comment.
This is missing fallback for RUS, but such doesn't exist for News page anyway. This is a bug @Rutjake
Made some fixes, so skeletonloader fits newselement better.
Reverting value to original
Rutjake
left a comment
Rutjake
left a comment
There was a problem hiding this comment.
Looks great and works well, nice job!
Using the Network Throttling feature in DevTools to simulate a slower connection is usually very helpful for catching these kinds of loading state issues.
For future improvements, we should investigate how Next.js loading.tsx could work within our project. Since we have the src/app structure, it might allow us to define loading UIs easily at the route level without having to modify the page code directly. It's a powerful pattern that could simplify how we handle these loading states moving forward.
3 participants
📄 Pull Request Overview
Closes #623
🔧 Changes Made
✅ Checklist Before Submission
console.log()or other debugging statements are left.📝 Additional Information
Provide any additional context or information that reviewers may need to know: