Conversation
Signed-off-by: Gašper Grom <gasper.grom@gmail.com>
Signed-off-by: Gašper Grom <gasper.grom@gmail.com>
Signed-off-by: Gašper Grom <gasper.grom@gmail.com>
Signed-off-by: Gašper Grom <gasper.grom@gmail.com>
Signed-off-by: Gašper Grom <gasper.grom@gmail.com>
WalkthroughThis pull request introduces several new Vue components for managing collection and project views, including filters, headers, detailed collections, and loading states. Enhancements include a revised scroll percentage calculation, dynamic dropdown bindings, and updated type definitions with additional project properties. New API endpoints for collections and projects are implemented, and styling is improved with new SCSS files and Tailwind configuration adjustments. Additionally, the Nuxt configuration now references a custom error component, and PrimeVue has been updated with a skeleton component for loading states. Changes
Possibly related PRs
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
frontend/app/components/uikit/skeleton/skeleton.stories.ts (1)
1-140:⚠️ Potential issueFile name doesn't match the component content.
This file is named
skeleton.stories.tsbut actually contains stories for theLfxButtoncomponent. The imports, component references, and all story definitions are for buttons, not skeletons. This mismatch will cause confusion for developers and potentially break documentation organization in Storybook.The file should either:
- Be renamed to
button.stories.tsto match its content, or- Be modified to actually contain stories for the
LfxSkeletoncomponent (as mentioned in the AI summary)If the intention was to create stories for a skeleton component, the content needs to be completely revised to import and showcase the skeleton component instead of buttons.
🧹 Nitpick comments (17)
frontend/app/error.vue (3)
5-9: Error handling looks great, consider using a more descriptive messageThe error display is clean and user-friendly. For non-404 errors, you might want to provide slightly more context about what might have gone wrong or what users can do besides trying again later.
20-20: Use the standard Nuxt import pathThe import from "nuxt/app" is unusual. Consider using the more standard import path:
-import {clearError, useRoute} from "nuxt/app"; +import {clearError, useRoute} from "#app";This ensures compatibility with Nuxt's module resolution system.
25-27: Improve type safety with a specific error interfaceThe error prop is currently typed as a generic "object", which lacks specificity. Consider defining a more specific interface for better type safety:
defineProps<{ - error: object + error: { + statusCode: number; + message?: string; + statusMessage?: string; + data?: any; + } }>()This will provide better TypeScript support when working with the error object.
frontend/app/components/uikit/skeleton/skeleton.vue (1)
1-13: Well-structured skeleton component wrapper.The component effectively wraps the PrimeVue skeleton component with proper attribute forwarding using
v-bind="$attrs". It follows the project's naming convention and uses Vue 3's composition API syntax.Consider removing the empty
<script setup>block if it's not needed, since it doesn't contain any imports, props declarations, or other logic:-<script setup lang="ts"> -</script>However, if maintaining this empty block is a project convention for consistency, it's fine to keep it.
frontend/app/components/modules/project/components/list/project-list-item-loading.vue (1)
20-29: Consider consolidating script sections.While having separate script sections is valid, consider consolidating them into a single script setup section for better maintainability, following modern Vue 3 patterns:
<script lang="ts" setup> import LfxCard from "~/components/uikit/card/card.vue"; import LfxSkeleton from "~/components/uikit/skeleton/skeleton.vue"; + defineOptions({ + name: 'LfxProjectListItemLoading' + }); </script> - <script lang="ts"> - export default { - name: 'LfxProjectListItemLoading' - } - </script>frontend/server/api/collections/[slug].ts (1)
26-26: Handle empty logo URLs properly.Some project logos have empty strings which might cause UI issues if not handled properly by the frontend components. Consider using null instead of empty strings for consistency with the Project interface, or provide default placeholder images.
- logo: '' + logo: nullfrontend/app/components/shared/utils/scroll.ts (1)
8-12: Improved scroll percentage calculation logic.The new calculation correctly accounts for the viewable area by subtracting the client height from the scroll height, which is the standard way to calculate scroll percentage. This will provide more accurate scroll position tracking.
Consider adding an explicit check to prevent division by zero when scrollHeight equals clientHeight:
const scrollTopPercentage = computed(() => { const scrollHeight = body?.scrollHeight || 1; const clientHeight = body?.clientHeight || 0; + const scrollableHeight = scrollHeight - clientHeight; + if (scrollableHeight <= 0) return 0; - return (scrollTop.value / (scrollHeight - clientHeight)) * 100; + return (scrollTop.value / scrollableHeight) * 100; });frontend/app/components/modules/collection/components/details/filters.vue (1)
61-61: Fix typo in sort option label.There's a spelling error in the sort option label "Alphabeticly".
- label: 'Alphabeticly', + label: 'Alphabetically',frontend/app/components/modules/collection/components/details/header.vue (4)
4-4: Fix spacing in class attribute.There's an extra space between the inline class and the dynamic class binding.
- <div class="flex gap-4 transition-all" :class="scrollTop > 50 ? 'py-4' : 'py-8'"> + <div class="flex gap-4 transition-all" :class="scrollTop > 50 ? 'py-4' : 'py-8'">
6-6: Remove empty class attribute.The empty class attribute isn't needed.
- <lfx-icon-button type="transparent" icon="angle-left" class="" /> + <lfx-icon-button type="transparent" icon="angle-left" />
46-46: Use v-else-if consistently across sibling conditionals.When using conditional rendering with v-if/v-else structures, it's better to use v-else-if for all middle conditions rather than mixing v-if and v-else.
- <span v-else>Contributors</span> + <span v-else-if="true">Contributors</span> - <span v-else>Organizations</span> + <span v-else-if="true">Organizations</span> - <span v-else>Projects</span> + <span v-else-if="true">Projects</span> - <span v-else>Software value</span> + <span v-else-if="true">Software value</span>Also applies to: 65-65, 85-85, 105-105
46-46: Consider consistent approach for formatting numbers.The component uses
formatNumberShortin some places and not in others. For consistency, consider using the same formatting function throughout the component.frontend/app/components/modules/project/components/list/project-list-item.vue (2)
46-46: Use consistent import paths.The component mixes relative paths and alias imports. For consistency and maintainability, consider using the alias import style throughout.
-import {formatNumber, formatNumberShort} from "../../../../shared/utils/formatter"; +import {formatNumber, formatNumberShort} from "~/components/shared/utils/formatter";
8-10: Add fallback for missing description.The component doesn't handle the case where a project description might be missing. Consider adding a fallback message or conditional rendering.
<p class="pt-3 text-body-1 text-neutral-500 line-clamp-2"> - {{ props.project.description }} + {{ props.project.description || 'No description available' }} </p>frontend/app/pages/collection/[slug].vue (1)
2-2: Consider adding a loading state.The component doesn't show any loading indicator while fetching the collection data. Consider adding a loading state to improve the user experience.
<template> + <div v-if="pending" class="container py-10"> + <lfx-skeleton height="15rem" width="100%" class="rounded-md" /> + </div> <lfx-collection-details-view v-if="data" :collection="data" /> </template>And update the script:
-const {data} = await useFetch<Collection>( +const {data, pending} = await useFetch<Collection>( () => `/api/collections/${slug}`, );frontend/app/components/uikit/skeleton/skeleton.stories.ts (2)
98-99: Improve template string formatting.The conditional expression is split across two lines in an awkward way, making it harder to read and maintain.
- <i :class="buttonProps.loading ? - 'fa-sharp fa-light fa-spinner-third animate-spin' : 'fa-solid fa-chart-scatter'" /> + <i :class="buttonProps.loading + ? 'fa-sharp fa-light fa-spinner-third animate-spin' + : 'fa-solid fa-chart-scatter'" />
94-109: Consider extracting custom template to a separate file.The custom template is large and makes the story file harder to read. For better maintainability, consider extracting it to a separate file or at least moving it to the bottom of the file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
frontend/app/assets/images/icon-gray.svgis excluded by!**/*.svgfrontend/app/assets/images/icon.svgis excluded by!**/*.svg
📒 Files selected for processing (20)
frontend/app/components/modules/collection/components/details/filters.vue(1 hunks)frontend/app/components/modules/collection/components/details/header.vue(1 hunks)frontend/app/components/modules/collection/views/collection-details.vue(1 hunks)frontend/app/components/modules/project/components/list/project-list-item-loading.vue(1 hunks)frontend/app/components/modules/project/components/list/project-list-item.vue(1 hunks)frontend/app/components/modules/project/types/project.ts(1 hunks)frontend/app/components/shared/utils/scroll.ts(1 hunks)frontend/app/components/uikit/dropdown/dropdown.vue(1 hunks)frontend/app/components/uikit/index.scss(1 hunks)frontend/app/components/uikit/skeleton/skeleton.scss(1 hunks)frontend/app/components/uikit/skeleton/skeleton.stories.ts(1 hunks)frontend/app/components/uikit/skeleton/skeleton.vue(1 hunks)frontend/app/components/uikit/tabs/tabs.scss(1 hunks)frontend/app/error.vue(1 hunks)frontend/app/pages/collection/[slug].vue(1 hunks)frontend/nuxt.config.ts(1 hunks)frontend/server/api/collections/[slug].ts(1 hunks)frontend/server/api/projects/index.ts(1 hunks)frontend/setup/primevue.ts(1 hunks)frontend/tailwind.config.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/app/components/uikit/skeleton/skeleton.scss
🔇 Additional comments (21)
frontend/nuxt.config.ts (1)
13-14: Configuration syntax looks goodAdding the custom error handler will improve the user experience when errors occur. The comma after the
headproperty was also correctly added.frontend/app/error.vue (1)
31-33: Good implementation of route watchingUsing a watcher to clear errors when the route changes is a good approach to prevent stale error states. This ensures users won't see error pages if they navigate away and back.
frontend/app/components/uikit/tabs/tabs.scss (1)
9-12: Good addition of the content width styling.Adding the
w-maxutility to the toggle button content ensures that the text will have appropriate width based on its content, preventing truncation and improving user experience.frontend/app/components/uikit/index.scss (1)
11-11: Properly added skeleton component import.The import for the skeleton styles is added in the correct alphabetical position, maintaining the file's organization.
frontend/tailwind.config.js (1)
77-77: Good addition of custom spacing value.Adding the
13: '3.25rem'spacing value extends Tailwind's utility classes in a consistent way, following the existing pattern and numerical ordering.frontend/setup/primevue.ts (1)
13-14: Good addition of the Skeleton component.The Skeleton component has been properly registered in the PrimeVue configuration, which will allow it to be used for loading states throughout the application. This is a good UX practice to provide visual feedback during data fetching.
frontend/app/components/modules/project/components/list/project-list-item-loading.vue (1)
1-23: Well-implemented loading skeleton component.The component follows good UX patterns by providing a placeholder that matches the structure of the actual content. The skeleton elements are properly sized and positioned to create a smooth loading experience.
frontend/server/api/collections/[slug].ts (2)
21-21: Duplicate image URL used for different projects.The same image URL is used for both Kubernetes and Prometheus, which would confuse users. Each project should have a unique logo.
58-65: Good implementation of the API endpoint.The endpoint correctly handles retrieving collection data by slug and properly handles the case where a collection is not found with an appropriate 404 error. The implementation follows Nuxt server API conventions.
frontend/app/components/modules/project/types/project.ts (1)
8-12: Good enhancement of the Project interface.Adding these properties enriches the Project type with valuable information that can be displayed in the UI. The types are appropriately defined, and this change aligns well with the data structure used in the collections API.
frontend/app/components/uikit/dropdown/dropdown.vue (1)
19-22: Improved binding of boolean properties.The change from string literals (
prop="true") to Vue bindings (:prop="true") is correct. This ensures the properties receive actual boolean values rather than string values, which can prevent potential type-related issues and explicitly shows these are dynamic values.frontend/app/components/modules/collection/components/details/filters.vue (1)
4-24: Well-structured component with responsive design.The component is well-designed with:
- Responsive padding based on scroll position
- Clear separation between tabs and sorting dropdown
- Proper use of computed properties for two-way binding
The integration with the LfxDropdown component and useScroll utility is implemented correctly.
frontend/app/components/modules/collection/components/details/header.vue (2)
134-137: LGTM! Props are properly typed with TypeScript.The component correctly uses TypeScript for prop definitions with proper optional types.
139-139: LGTM! Good use of scroll utility.The scroll utility is correctly used to make the header responsive based on scroll position.
frontend/app/components/modules/collection/views/collection-details.vue (2)
9-11: LGTM! Good conditional rendering for data and loading states.The component correctly handles the different loading states and data availability.
28-30: LGTM! Props are properly typed.The component correctly uses TypeScript for prop definitions.
frontend/app/components/modules/project/components/list/project-list-item.vue (2)
1-42: LGTM! Well-structured project card with clear layout.The component has a clean and well-organized structure that clearly displays the project's key information.
54-56: LGTM! Props are properly typed.The component correctly uses TypeScript for prop definitions.
frontend/app/pages/collection/[slug].vue (1)
15-31: LGTM! Excellent error handling for 404 cases.The component handles 404 errors appropriately, with different approaches for server and client-side rendering.
frontend/app/components/uikit/skeleton/skeleton.stories.ts (2)
101-102: Inconsistent loading state handling in custom template.In the first button, you're showing a spinner when loading, but the second icon (
fa-chart-scatter) is always displayed regardless of loading state. This creates inconsistent UX as loading buttons should typically replace all icons with loading indicators or disable them.
4-50: LGTM: Well-structured Storybook configuration.The Storybook configuration is well-structured with comprehensive argTypes definitions and proper control types for each property. The background parameters provide good theme testing options.
| name: 'CrowdKube', | ||
| slug: 'crowdKube', | ||
| logo: '' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure consistent slug casing.
Project slugs use inconsistent casing patterns. 'crowdKube' uses camelCase while other slugs like 'kubernetes' use lowercase. This could cause issues with URL matching. For best practices with URLs, use consistent lowercase kebab-case for all slugs.
- slug: 'crowdKube',
+ slug: 'crowd-kube',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| name: 'CrowdKube', | |
| slug: 'crowdKube', | |
| logo: '' | |
| name: 'CrowdKube', | |
| - slug: 'crowdKube', | |
| + slug: 'crowd-kube', | |
| logo: '' |
| { | ||
| name: 'Kubernetes', | ||
| slug: 'kubernetes', | ||
| logo: 'https://c8.alamy.com/comp/2M8NCEE/kubernetes-logo-white-background-2M8NCEE.jpg' |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid using external image URLs in mock data.
Using external image URLs from commercial stock photo sites (alamy.com) in mock data is problematic as these will likely be replaced in production. Consider using placeholder images that are locally available or use consistent placeholder services like placehold.co.
🏁 Script executed:
#!/bin/bash
# Count how many external URLs are used in mock data
grep -c "alamy.com" frontend/server/api/collections/\[slug\].tsLength of output: 65
Update Mock Data to Improve Robustness
- Avoid external image URLs: The mock data includes 2 occurrences of external image URLs (alamy.com). Relying on these can lead to broken links or licensing issues in production. Please replace them with locally available placeholder images—or use a reliable placeholder service like placehold.co.
- Handle empty logos: Some collections have logo fields set as empty strings. This might cause UI issues. Ensure every collection has a valid image URL, even if it's a consistent placeholder.
- Normalize slug casing: Collection slugs are inconsistently cased (e.g., 'crowdKube', 'crowdSQL', 'crowdML' use camelCase versus other slugs being lowercase). Standardize the casing (preferably lowercase) to maintain URL matching consistency.
- Distinct logo images: The Kubernetes and Prometheus collections currently use the same image URL. Verify if this is intentional; if not, update one to reflect the correct logo.
| export default defineEventHandler(async (event) => { | ||
| const sort: string = getQuery(event)?.sort as string || ''; | ||
| const page: string = getQuery(event)?.page as string || ''; | ||
| const pageSize: string = getQuery(event)?.pageSize as string || ''; | ||
| // const isLfx: boolean = getQuery(event)?.isLfx === true || false; | ||
| const [field, order] = sort.split('_'); | ||
| return { | ||
| page: +page, | ||
| pageSize: +pageSize, | ||
| total: 2, | ||
| data: projects.sort((a, b) => { | ||
| const fieldA = a[field as keyof Project]; | ||
| const fieldB = b[field as keyof Project]; | ||
| if (fieldA < fieldB) { | ||
| return order === 'ASC' ? -1 : 1; | ||
| } | ||
| if (fieldA > fieldB) { | ||
| return order === 'ASC' ? 1 : -1; | ||
| } | ||
| return 0; | ||
| }) | ||
| }; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation and error handling to the API endpoint.
The API implementation is functional but lacks error handling and validation for the sort field. Additionally, there's a hardcoded total value and a commented-out filter.
- Add validation for the sort field:
export default defineEventHandler(async (event) => {
const sort: string = getQuery(event)?.sort as string || '';
const page: string = getQuery(event)?.page as string || '';
const pageSize: string = getQuery(event)?.pageSize as string || '';
// const isLfx: boolean = getQuery(event)?.isLfx === true || false;
const [field, order] = sort.split('_');
+
+ // Validate sort field
+ const validFields: (keyof Project)[] = ['name', 'contributorsCount', 'organizationsCount', 'softwareValueCount'];
+ const validField = field && validFields.includes(field as keyof Project) ? field : 'name';
+ const validOrder = order === 'ASC' ? 'ASC' : 'DESC';
+
+ // Apply filters
+ let filteredProjects = [...projects];
+ // if (getQuery(event)?.isLfx === 'true') {
+ // filteredProjects = filteredProjects.filter(p => p.isLfx);
+ // }
return {
page: +page,
pageSize: +pageSize,
- total: 2,
+ total: filteredProjects.length,
- data: projects.sort((a, b) => {
+ data: filteredProjects.sort((a, b) => {
- const fieldA = a[field as keyof Project];
- const fieldB = b[field as keyof Project];
+ const fieldA = a[validField as keyof Project];
+ const fieldB = b[validField as keyof Project];
if (fieldA < fieldB) {
- return order === 'ASC' ? -1 : 1;
+ return validOrder === 'ASC' ? -1 : 1;
}
if (fieldA > fieldB) {
- return order === 'ASC' ? 1 : -1;
+ return validOrder === 'ASC' ? 1 : -1;
}
return 0;
})
};
});- Decide whether to implement the
isLfxfilter or remove the commented code.
Is filtering by isLfx supposed to be implemented? The commented-out line suggests it was intended but not completed.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export default defineEventHandler(async (event) => { | |
| const sort: string = getQuery(event)?.sort as string || ''; | |
| const page: string = getQuery(event)?.page as string || ''; | |
| const pageSize: string = getQuery(event)?.pageSize as string || ''; | |
| // const isLfx: boolean = getQuery(event)?.isLfx === true || false; | |
| const [field, order] = sort.split('_'); | |
| return { | |
| page: +page, | |
| pageSize: +pageSize, | |
| total: 2, | |
| data: projects.sort((a, b) => { | |
| const fieldA = a[field as keyof Project]; | |
| const fieldB = b[field as keyof Project]; | |
| if (fieldA < fieldB) { | |
| return order === 'ASC' ? -1 : 1; | |
| } | |
| if (fieldA > fieldB) { | |
| return order === 'ASC' ? 1 : -1; | |
| } | |
| return 0; | |
| }) | |
| }; | |
| }); | |
| export default defineEventHandler(async (event) => { | |
| const sort: string = getQuery(event)?.sort as string || ''; | |
| const page: string = getQuery(event)?.page as string || ''; | |
| const pageSize: string = getQuery(event)?.pageSize as string || ''; | |
| // const isLfx: boolean = getQuery(event)?.isLfx === true || false; | |
| const [field, order] = sort.split('_'); | |
| // Validate sort field | |
| const validFields: (keyof Project)[] = ['name', 'contributorsCount', 'organizationsCount', 'softwareValueCount']; | |
| const validField = field && validFields.includes(field as keyof Project) ? field : 'name'; | |
| const validOrder = order === 'ASC' ? 'ASC' : 'DESC'; | |
| // Apply filters | |
| let filteredProjects = [...projects]; | |
| // if (getQuery(event)?.isLfx === 'true') { | |
| // filteredProjects = filteredProjects.filter(p => p.isLfx); | |
| // } | |
| return { | |
| page: +page, | |
| pageSize: +pageSize, | |
| total: filteredProjects.length, | |
| data: filteredProjects.sort((a, b) => { | |
| const fieldA = a[validField as keyof Project]; | |
| const fieldB = b[validField as keyof Project]; | |
| if (fieldA < fieldB) { | |
| return validOrder === 'ASC' ? -1 : 1; | |
| } | |
| if (fieldA > fieldB) { | |
| return validOrder === 'ASC' ? 1 : -1; | |
| } | |
| return 0; | |
| }) | |
| }; | |
| }); |
| const { data, status } = useFetch<Pagination<Project>>( | ||
| () => `/api/projects`, | ||
| { | ||
| params: { | ||
| sort, | ||
| page, | ||
| pageSize, | ||
| isLfx: tab.value === 'lfx', | ||
| }, | ||
| watch: [sort, tab, page], | ||
| transform: (res: Pagination<Project>) => { | ||
| if (res.page === 1) { | ||
| projects.value = res.data; | ||
| } else { | ||
| projects.value = [...projects.value, ...res.data]; | ||
| } | ||
| return { | ||
| ...res, | ||
| data: projects.value, | ||
| }; | ||
| }, | ||
| } | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for the API request.
The component doesn't handle API request failures. Consider adding error handling to provide a better user experience when API requests fail.
const { data, status, error } = useFetch<Pagination<Project>>(
() => `/api/projects`,
{
params: {
sort,
page,
pageSize,
isLfx: tab.value === 'lfx',
},
watch: [sort, tab, page],
transform: (res: Pagination<Project>) => {
if (res.page === 1) {
projects.value = res.data;
} else {
projects.value = [...projects.value, ...res.data];
}
return {
...res,
data: projects.value,
};
},
}
);
+ watch(error, (newError) => {
+ if (newError) {
+ console.error('Failed to fetch projects:', newError);
+ // Consider showing an error message to the user
+ }
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { data, status } = useFetch<Pagination<Project>>( | |
| () => `/api/projects`, | |
| { | |
| params: { | |
| sort, | |
| page, | |
| pageSize, | |
| isLfx: tab.value === 'lfx', | |
| }, | |
| watch: [sort, tab, page], | |
| transform: (res: Pagination<Project>) => { | |
| if (res.page === 1) { | |
| projects.value = res.data; | |
| } else { | |
| projects.value = [...projects.value, ...res.data]; | |
| } | |
| return { | |
| ...res, | |
| data: projects.value, | |
| }; | |
| }, | |
| } | |
| ); | |
| const { data, status, error } = useFetch<Pagination<Project>>( | |
| () => `/api/projects`, | |
| { | |
| params: { | |
| sort, | |
| page, | |
| pageSize, | |
| isLfx: tab.value === 'lfx', | |
| }, | |
| watch: [sort, tab, page], | |
| transform: (res: Pagination<Project>) => { | |
| if (res.page === 1) { | |
| projects.value = res.data; | |
| } else { | |
| projects.value = [...projects.value, ...res.data]; | |
| } | |
| return { | |
| ...res, | |
| data: projects.value, | |
| }; | |
| }, | |
| } | |
| ); | |
| watch(error, (newError) => { | |
| if (newError) { | |
| console.error('Failed to fetch projects:', newError); | |
| // Consider showing an error message to the user | |
| } | |
| }); |
| watch(scrollTopPercentage, () => { | ||
| if (scrollTopPercentage.value >= 100 && projects.value.length < data.value.total) { | ||
| page.value += 1; | ||
| projects.value = []; | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix pagination logic to avoid UI flickering.
When scrollTopPercentage reaches 100%, you're incrementing the page but also resetting the projects array. This could cause UI flickering as existing projects disappear momentarily before new ones load.
watch(scrollTopPercentage, () => {
if (scrollTopPercentage.value >= 100 && projects.value.length < data.value.total) {
page.value += 1;
- projects.value = [];
}
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| watch(scrollTopPercentage, () => { | |
| if (scrollTopPercentage.value >= 100 && projects.value.length < data.value.total) { | |
| page.value += 1; | |
| projects.value = []; | |
| } | |
| }); | |
| watch(scrollTopPercentage, () => { | |
| if (scrollTopPercentage.value >= 100 && projects.value.length < data.value.total) { | |
| page.value += 1; | |
| } | |
| }); |
Signed-off-by: Gašper Grom <gasper.grom@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
frontend/app/components/modules/collection/views/collection-details.vue (2)
70-75:⚠️ Potential issueFix pagination logic to avoid UI flickering.
When scrollTopPercentage reaches 100%, you're incrementing the page but also resetting the projects array. This will cause UI flickering as existing projects disappear before new ones load.
watch(scrollTopPercentage, () => { if (scrollTopPercentage.value >= 100 && projects.value.length < data.value.total) { page.value += 1; - projects.value = []; } });
46-68: 🛠️ Refactor suggestionAdd error handling for the API request.
The component doesn't handle API request failures. Consider adding error handling to provide a better user experience when API requests fail.
-const { data, status } = await useFetch<Pagination<Project>>( +const { data, status, error } = await useFetch<Pagination<Project>>( () => `/api/projects`, { params: { sort, page, pageSize, isLfx: tab.value === 'lfx', }, watch: [sort, tab, page], transform: (res: Pagination<Project>) => { if (res.page === 1) { projects.value = res.data; } else { projects.value = [...projects.value, ...res.data]; } return { ...res, data: projects.value, }; }, } ); +watch(error, (newError) => { + if (newError) { + console.error('Failed to fetch projects:', newError); + // Consider showing an error message to the user + } +});
🧹 Nitpick comments (3)
frontend/app/components/modules/collection/views/collection-details.vue (3)
8-15: Add empty state handling when no projects are available.The component doesn't handle the case when there are no projects to display. Consider adding an empty state message when the data is loaded but there are no projects.
<div class="container py-10 flex flex-col gap-8"> <div v-if="data && !(status === 'pending' && data?.page === 1)" class="grid grid-cols-3 gap-8"> <lfx-project-list-item v-for="project of data?.data" :key="project.slug" :project="project" /> </div> <div v-if="status === 'pending'" class="grid grid-cols-3 gap-8"> <lfx-project-list-item-loading v-for="i of 6" :key="i" /> </div> + <div v-if="data?.data?.length === 0 && status === 'success'" class="flex flex-col items-center justify-center py-10"> + <p class="text-lg text-gray-500">No projects found in this collection.</p> + </div> </div>
2-7: Apply semantic HTML elements for better accessibility.Consider using semantic HTML elements like
headerinstead ofdivfor your header section to improve accessibility and SEO.- <div class=" min-h-[12.875rem] sticky top-14 lg:top-17 z-10"> - <div class="bg-white outline outline-neutral-100"> + <header class="min-h-[12.875rem] sticky top-14 lg:top-17 z-10"> + <div class="bg-white outline outline-neutral-100"> <lfx-collection-header :loading="!collection" :collection="props.collection" /> <lfx-collection-filters v-model:sort="sort" v-model:tab="tab" /> </div> - </div> + </header>
77-79: Add conditional check before accessing data.value.data.The onMounted hook may be triggered before the data is fetched, resulting in a potential error when accessing data.value.data.
onMounted(() => { - projects.value = data.value?.data || []; + if (data.value) { + projects.value = data.value.data || []; + } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/app/components/modules/collection/views/collection-details.vue(1 hunks)
🔇 Additional comments (1)
frontend/app/components/modules/collection/views/collection-details.vue (1)
46-53: Add collection ID or slug to the API request.The component fetches projects but doesn't filter them by the collection. Consider adding the collection ID or slug to the API request parameters to fetch only the projects related to the current collection.
const { data, status } = await useFetch<Pagination<Project>>( () => `/api/projects`, { params: { sort, page, pageSize, isLfx: tab.value === 'lfx', + collectionId: props.collection?.id || props.collection?.slug, }, watch: [sort, tab, page],Do you need to filter projects by collection ID or slug? If so, please confirm the proper parameter name for the API endpoint.
| <div v-if="data && !(status === 'pending' && data?.page === 1)" class="grid grid-cols-3 gap-8"> | ||
| <lfx-project-list-item v-for="project of data?.data" :key="project.slug" :project="project" /> | ||
| </div> | ||
| <div v-if="status === 'pending'" class="grid grid-cols-3 gap-8"> | ||
| <lfx-project-list-item-loading v-for="i of 6" :key="i" /> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve responsive design for the grid layout.
The grid is set to 3 columns without any responsive adjustments for smaller screens. This may lead to poor display on mobile and tablet devices.
- <div v-if="data && !(status === 'pending' && data?.page === 1)" class="grid grid-cols-3 gap-8">
+ <div v-if="data && !(status === 'pending' && data?.page === 1)" class="grid grid-cols-1 md:grid-cols-2 lg:grid-cols-3 gap-8">
<lfx-project-list-item v-for="project of data?.data" :key="project.slug" :project="project" />
</div>
- <div v-if="status === 'pending'" class="grid grid-cols-3 gap-8">
+ <div v-if="status === 'pending'" class="grid grid-cols-1 md:grid-cols-2 lg:grid-cols-3 gap-8">
<lfx-project-list-item-loading v-for="i of 6" :key="i" />
</div>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div v-if="data && !(status === 'pending' && data?.page === 1)" class="grid grid-cols-3 gap-8"> | |
| <lfx-project-list-item v-for="project of data?.data" :key="project.slug" :project="project" /> | |
| </div> | |
| <div v-if="status === 'pending'" class="grid grid-cols-3 gap-8"> | |
| <lfx-project-list-item-loading v-for="i of 6" :key="i" /> | |
| </div> | |
| <div v-if="data && !(status === 'pending' && data?.page === 1)" class="grid grid-cols-1 md:grid-cols-2 lg:grid-cols-3 gap-8"> | |
| <lfx-project-list-item v-for="project of data?.data" :key="project.slug" :project="project" /> | |
| </div> | |
| <div v-if="status === 'pending'" class="grid grid-cols-1 md:grid-cols-2 lg:grid-cols-3 gap-8"> | |
| <lfx-project-list-item-loading v-for="i of 6" :key="i" /> | |
| </div> |
2 participants
Summary by CodeRabbit
New Features
Style/UI Enhancements