Popover component and project management fixes #2732
Conversation
WalkthroughThis pull request represents a significant refactoring of the frontend module structure, primarily focusing on reorganizing components from the Changes
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 🪧 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: 6
🔭 Outside diff range comments (2)
frontend/src/assets/scss/layout.scss (1)
Line range hint
84-91: Consider a more maintainable approach for autocomplete stylingThe current implementation uses a very long transition duration (5000s) as a workaround for browser autocomplete styling. Consider:
- Using a more standard approach with form styling
- Letting autoprefixer handle vendor prefixes
- Using a shorter transition duration if the effect is still needed
input:-webkit-autofill, input:-webkit-autofill:hover, input:-webkit-autofill:focus, input:-webkit-autofill:active { - transition: background-color 5000s ease-in-out 0s; + transition: background-color 0s; + background-color: transparent !important; -webkit-text-fill-color: #000 !important; }frontend/src/modules/organization/components/edit/organization-add.vue (1)
Line range hint
213-238: Enhance error handling for organization creationThe error handling for organization creation could be more specific and provide better user feedback.
OrganizationApiService.create(data, [form.subproject]) .then((newOrg: Organization) => { router.push({ name: 'organizationView', params: { id: newOrg.id, }, query: { projectGroup: selectedProjectGroup.value?.id, }, }); isModalOpen.value = false; resetForm(); }) .catch((error: any) => { - if (error.response.status === 409) { - Message.error('Organization was not created because the identity already exists in another profile'); - } else { - Errors.handle(error); - } + const errorMessages = { + 409: 'Organization was not created because the identity already exists in another profile', + 400: 'Invalid organization data provided', + 403: 'You do not have permission to create organizations', + default: 'An error occurred while creating the organization' + }; + const status = error.response?.status; + Message.error(errorMessages[status] || errorMessages.default); + Errors.handle(error); })
🧹 Nitpick comments (29)
frontend/src/ui-kit/pill/types/PillType.ts (1)
1-3: Consider adding JSDoc documentation.The new semantic types would benefit from clear documentation explaining their intended usage and visual appearance.
Add documentation like this:
+/** + * Predefined pill types for semantic styling + * - primary: Main action or status + * - secondary: Alternative or supporting status + * - tertiary: Less prominent status + * - success: Positive or completed status + * - warning: Cautionary status + * - transparent: Subtle or background status + */ export const pillType = ['primary', 'secondary', 'tertiary', 'success', 'warning', 'transparent'] as const; +/** Type representing the available pill variants */ export type PillType = (typeof pillType)[number]frontend/src/assets/scss/layout.scss (1)
Line range hint
65-73: Consider standardizing spinner implementations and using design system variablesThe custom spinner implementation could be improved by:
- Using design system color variables instead of hardcoded values
- Standardizing units (px vs rem)
- Considering consolidation with existing Element Plus spinner styles
.custom-spinner::before { - @apply border-2 border-gray-100 border-b-gray-900 border-x-gray-900 rounded-full h-8 w-8 absolute; + @apply border-2 border-gray-100 border-b-primary border-x-primary rounded-full h-8 w-8 absolute; top: 10%; left: 10%; transform: translate3d(-50%, -50%, 0); content: ""; }frontend/src/ui-kit/popover/popover.scss (1)
1-6: Consider adding fallback for w-fit propertyThe
w-fitproperty has limited browser support. Consider adding a fallback for older browsers:.c-popover { - @apply relative w-fit flex; + @apply relative w-auto w-fit-content w-fit flex; } &__trigger { - @apply w-fit; + @apply w-auto w-fit-content w-fit; }frontend/src/ui-kit/dropdown/Dropdown.vue (1)
2-4: Consider adding an accessible trigger element.
Currently, the trigger slot may be replaced by arbitrary content, which might not always be keyboard-accessible. Adopting a button or anchor element as the trigger could improve accessibility and semantics.frontend/src/ui-kit/popover/Popover.vue (3)
1-24: Enhance keyboard accessibility for the popover.
Currently, the template focuses on clicks and hover, but it does not address how keyboard users (e.g., pressing Enter or Escape) interact with the popover. Consider implementing keyboard navigation (e.g., close on “Esc” key) to improve accessibility.
26-45: Validate default prop values.
The default prop values such as spacing=4 might need refining in different layouts or themes. Regularly confirm that these defaults provide an optimal user experience across various devices.
61-65: Consider merging toggle logic with show/hide methods.
Instead of duplicating logic, a consolidated approach may reduce complexity and avoid future maintenance errors. For example, you can unify “togglePopover” with “showPopover”/“hidePopover” logic to handle outside click listeners more seamlessly.frontend/src/ui-kit/popover/types/PopoverTrigger.ts (1)
1-6: Good use of literal types for 'PopoverTrigger'.
Defining an array of acceptable trigger types and using an indexed access union enforces type safety. For clarity, consider adding short docstrings describing each trigger type.frontend/src/ui-kit/popover/types/PopoverPlacement.ts (1)
1-16: Provide fallback or boundary handling for popover placements.
While enumerating valid placements is helpful, ensure you handle or provide fallbacks for scenarios where there isn’t enough space in the chosen direction (e.g., using collision detection or boundary adjustment).Would you like help implementing an automated fallback mechanism for limited space scenarios?
frontend/src/ui-kit/pill/Pill.vue (1)
3-6: Update component name to match new prefixThe component uses
c-pillclass prefix but is still namedLfPill. Consider updating the component name to maintain consistency:export default { - name: 'LfPill', + name: 'CPill', };Also applies to: 22-24
frontend/src/modules/admin/modules/projects/components/fragments/lf-project-count.vue (1)
Line range hint
20-22: Consider updating component prefixThe component name still uses the
Lfprefix while the codebase seems to be moving away from this convention. Consider updating for consistency with other modernized components.frontend/src/modules/admin/modules/projects/components/fragments/lf-status-pill.vue (1)
2-4: Add error handling for invalid status valuesThe fallback to 'primary' type is good, but consider logging invalid status values for debugging:
- <lf-pill :type="statusDisplay[props.status]?.type || 'primary'"> - {{ statusDisplay[props.status]?.label }} - </lf-pill> + <lf-pill :type="getStatusType(props.status)"> + {{ getStatusLabel(props.status) }} + </lf-pill> + // Add to script section: + const getStatusType = (status: string) => { + if (!statusDisplay[status]) { + console.warn(`Invalid status: ${status}`); + } + return statusDisplay[status]?.type || 'primary'; + }; + + const getStatusLabel = (status: string) => { + return statusDisplay[status]?.label || status; + };frontend/src/modules/admin/modules/projects/components/fragments/lf-project-column.vue (1)
Line range hint
3-23: Consider enhancing popover accessibility.The el-popover implementation could benefit from improved accessibility:
- Add
aria-labelto describe the popover content- Add keyboard navigation support for users who can't use hover
<el-popover placement="top" :width="250" trigger="hover" + aria-label="Project list" + :trigger="['hover', 'focus']" > <template #reference> - <app-lf-project-count :count="projects.length" /> + <app-lf-project-count + :count="projects.length" + tabindex="0" + role="button" + /> </template>frontend/src/modules/admin/modules/projects/components/lf-sub-projects-dropdown.vue (1)
3-13: Enhance dropdown accessibility and UX.The icon-only button and dropdown item could benefit from accessibility improvements:
- <lf-dropdown placement="bottom-end" width="12rem"> + <lf-dropdown + placement="bottom-end" + width="12rem" + aria-label="Sub-project actions" + > <template #trigger> - <lf-button type="secondary-ghost-light" icon-only> + <lf-button + type="secondary-ghost-light" + icon-only + aria-label="Open sub-project actions" + > <lf-icon name="ellipsis" type="regular" /> </lf-button> </template> - <lf-dropdown-item @click="editSubProject()"> + <lf-dropdown-item + @click="editSubProject()" + @keydown.enter="editSubProject()" + > <lf-icon name="pen" /> Edit sub-project </lf-dropdown-item> </lf-dropdown>frontend/src/ui-kit/dropdown/dropdown.scss (1)
2-2: LGTM! Consider adding focus styles for keyboard navigationThe new styling provides good visual hierarchy and consistent spacing. The use of Tailwind's utility classes is appropriate.
Consider adding focus styles for keyboard navigation:
.c-dropdown { @apply border border-gray-100 shadow-md p-2 flex flex-col gap-1 rounded-xl bg-white; + &:focus-visible { + @apply outline-none ring-2 ring-primary-500; + }frontend/src/modules/admin/modules/projects/components/lf-projects-dropdown.vue (3)
38-43: Add TypeScript type annotationsConsider adding TypeScript type annotations for better type safety and developer experience.
defineProps({ id: { type: String, - default: null, + default: null, + required: false, }, } as const); + type Events = { + onEditProject: [] // no payload + onAddSubProject: [] // no payload + } - const emit = defineEmits(['onEditProject', 'onAddSubProject']); + const emit = defineEmits<Events>();
45-45: Consider using kebab-case for event namesVue's style guide recommends using kebab-case for event names.
- const emit = defineEmits(['onEditProject', 'onAddSubProject']); + const emit = defineEmits(['edit-project', 'add-sub-project']);
49-55: Consider adding error handling for emit actionsThe event handlers could benefit from try-catch blocks to handle potential errors.
const editProject = () => { + try { emit('onEditProject'); + } catch (error) { + console.error('Failed to emit edit project event:', error); + } }; const addSubProject = () => { + try { emit('onAddSubProject'); + } catch (error) { + console.error('Failed to emit add sub-project event:', error); + } };frontend/src/ui-kit/popover/Popover.stories.ts (1)
51-72: Consider enhancing the default story with more examples.While the current example is good, consider adding more stories to showcase different:
- Placements (top, bottom, left, right)
- Trigger events (hover, click)
- Content variations
Example addition:
export const Placements = { args: { ...Default.args, }, render: () => ({ components: { LfPopover, LfButton }, template: ` <div class="flex gap-4"> <lf-popover v-for="placement in popoverPlacements" :placement="placement"> <template #trigger> <lf-button>{{ placement }}</lf-button> </template> <div class="p-4">Popover content</div> </lf-popover> </div> `, }), };frontend/src/modules/admin/modules/projects/components/lf-project-groups-dropdown.vue (1)
9-35: Consider simplifying complex permission conditions.The nested permission checks in the template make it harder to maintain. Consider extracting these into computed properties.
Example refactor:
+ const canEdit = computed(() => + hasPermission(LfPermission.projectGroupEdit) && + hasAccessToSegmentId(id) + ); + + const canCreate = computed(() => + hasPermission(LfPermission.projectCreate) && + hasAccessToSegmentId(id) && + !showEditOnly + ); - v-if="hasPermission(LfPermission.projectGroupEdit) && hasAccessToSegmentId(id)" + v-if="canEdit" - v-if="hasPermission(LfPermission.projectCreate) && hasAccessToSegmentId(id) && !showEditOnly" + v-if="canCreate"frontend/src/modules/activity/pages/activity-list-page.vue (1)
Line range hint
96-100: Consider using an enum for drawer statesThe current implementation uses string literals for drawer states. Consider using an enum to improve type safety and maintainability.
+enum DrawerState { + ADD_ACTIVITY = 'add-activity', + ACTIVITY_TYPES = 'activity-types', +} -const drawer = ref<string | undefined>(); +const drawer = ref<DrawerState | undefined>(); -if (drawer.value === 'add-activity') { +if (drawer.value === DrawerState.ADD_ACTIVITY) { isActivityDrawerOpen.value = true; -} else if (drawer.value === 'activity-types') { +} else if (drawer.value === DrawerState.ACTIVITY_TYPES) { isActivityTypeDrawerOpen.value = true; }frontend/src/modules/admin/modules/projects/components/view/lf-projects-table.vue (2)
13-16: Consider extracting permission check to computed propertyThe template contains a complex permission check that could be moved to a computed property for better maintainability and reusability.
+const canAddSubProject = computed(() => + hasPermission(LfPermission.subProjectCreate) && + hasAccessToSegmentId(project.id) +); <lf-button - v-if=" - hasPermission(LfPermission.subProjectCreate) - && hasAccessToSegmentId(project.id) - " + v-if="canAddSubProject" type="secondary-ghost-light" icon-only @click="emit('onAddSubProject', project)" >
116-168: Consider using CSS custom properties for repeated valuesThe styling section contains repeated color and spacing values that could be extracted into CSS custom properties.
<style lang="scss"> +:root { + --projects-table-border-color: #e5e7eb; + --projects-table-padding-x: 1rem; + --projects-table-padding-y: 1rem; +} #projects-table { - @apply rounded-lg shadow-sm border border-solid border-gray-200; + @apply rounded-lg shadow-sm border border-solid; + border-color: var(--projects-table-border-color); thead .table-columns { - @apply align-middle h-auto px-2 py-4; + @apply align-middle h-auto; + padding: var(--projects-table-padding-y) var(--projects-table-padding-x);frontend/src/modules/admin/modules/projects/pages/project-groups-list.page.vue (1)
Line range hint
192-194: Consider adding retry logic for image loadingThe image error handling is basic and could be enhanced with retry logic or fallback URLs.
+const MAX_RETRY_ATTEMPTS = 3; +const imageRetryCount = reactive({}); const handleImageError = (id, e) => { + const currentRetry = imageRetryCount[id] || 0; + if (currentRetry < MAX_RETRY_ATTEMPTS) { + imageRetryCount[id] = currentRetry + 1; + // Retry with timestamp to bypass cache + e.target.src = `${projectGroup.url}?retry=${Date.now()}`; + } else { imageErrors[id] = true; + } };frontend/src/modules/organization/components/edit/organization-add.vue (1)
Line range hint
134-157: Consider using a type-safe approach for form validation rulesThe validation rules could benefit from stronger typing to catch potential issues at compile time.
+type ValidationRules = { + subproject: { required: true }; + name: { required: true }; + website: { required: true }; + identities: { + required: (value: OrganizationAddIdentity[]) => boolean; + }; +}; -const rules = { +const rules: ValidationRules = { subproject: { required, }, name: { required, }, website: { required, }, identities: { required: (value: OrganizationAddIdentity[]) => value.some((i) => i.value.length), }, };frontend/src/modules/organization/pages/organization-form-page.vue (4)
Line range hint
441-507: Consider refactoring the form submission logic for better maintainability.The
onSubmitfunction handles both creation and update scenarios with complex error handling. Consider splitting this into separate methods for better maintainability and testing.- async function onSubmit() { + async function handleOrganizationUpdate(payload) { + trackEvent({ + key: FeatureEventKey.EDIT_ORGANIZATION, + type: EventType.FEATURE, + properties: { + ...payload.values, + }, + }); + + try { + await OrganizationService.update(payload.id, payload.values); + Message.success(i18n('entities.organization.update.success')); + return true; + } catch (error) { + if (error.response.status === 409) { + Message.error( + h( + 'div', + { class: 'flex flex-col gap-2' }, + [ + h( + 'el-button', + { + class: 'btn btn--xs btn--secondary !h-6 !w-fit', + onClick: () => { + organizationsStore.addToMergeOrganizations(payload.id, error.response.data); + Message.closeAll(); + }, + }, + 'Merge organizations', + ), + ], + ), + { + title: 'Organization was not updated because the website already exists in another organization.', + }, + ); + } else { + Errors.handle(error); + } + return false; + } + } + + async function handleOrganizationCreate(payload) { + trackEvent({ + key: FeatureEventKey.ADD_ORGANIZATION, + type: EventType.FEATURE, + properties: { + ...payload, + }, + }); + + try { + await OrganizationService.create(payload); + Message.success(i18n('entities.organization.create.success')); + return true; + } catch (error) { + Message.error(i18n('entities.organization.create.error')); + Errors.handle(error); + return false; + } + } + + async function onSubmit() { isFormSubmitting.value = true; const { emails, ...rest } = formModel.value; const name = isEditPage.value === false ? formModel.value.displayName : undefined; const data = { manuallyCreated: true, ...rest, attributes: { name: { default: name, custom: [name], }, }, }; const payload = isEditPage.value ? { id: props.id, values: data, segments: segments.value, } : { ...data, segments: segments.value, }; - // Edit - if (isEditPage.value) { - trackEvent({...}) - try {...} catch (error) {...} - } else { - trackEvent({...}) - try {...} catch (error) {...} - } + const success = isEditPage.value + ? await handleOrganizationUpdate(payload) + : await handleOrganizationCreate(payload); isFormSubmitting.value = false; - wasFormSubmittedSuccessfully.value = true; + wasFormSubmittedSuccessfully.value = success; }
Line range hint
508-516: Consider extracting route query handling logic.The
onChangefunction directly manipulates route query parameters. Consider extracting this logic into a dedicated composable for better reusability and testing.+ function useRouteQueryManager() { + const router = useRouter(); + + const updateSubprojectQuery = (subprojectId) => { + router.replace({ + name: 'organizationCreate', + query: { + subprojectId, + }, + }); + }; + + return { + updateSubprojectQuery, + }; + } + - const onChange = ({ subprojectId }) => { - router.replace({ - name: 'organizationCreate', - query: { - subprojectId, - }, - }); - }; + const { updateSubprojectQuery } = useRouteQueryManager(); + const onChange = ({ subprojectId }) => { + updateSubprojectQuery(subprojectId); + };
Line range hint
471-486: Enhance error messages for better user experience.The error handling for organization conflicts provides good user feedback, but other error scenarios could be more specific to help users understand and resolve issues.
} catch (error) { if (error.response.status === 409) { Message.error( h( 'div', { class: 'flex flex-col gap-2' }, [ h( 'el-button', { class: 'btn btn--xs btn--secondary !h-6 !w-fit', onClick: () => { organizationsStore.addToMergeOrganizations(payload.id, error.response.data); Message.closeAll(); }, }, 'Merge organizations', ), ], ), { title: 'Organization was not updated because the website already exists in another organization.', }, ); + } else if (error.response.status === 400) { + Message.error(i18n('entities.organization.update.validationError'), { + title: 'Validation Error', + description: error.response.data.message || 'Please check your input and try again.', + }); + } else if (error.response.status === 403) { + Message.error(i18n('entities.organization.update.permissionError'), { + title: 'Permission Error', + description: 'You do not have permission to update this organization.', + }); } else { Errors.handle(error); }
Line range hint
529-559: Consider using CSS custom properties for better maintainability.The SCSS styles could be more maintainable by using CSS custom properties for common values and leveraging Element Plus's built-in customization options.
<style lang="scss"> + :root { + --organization-form-spacing: 1.5rem; + --organization-form-btn-gap: 0.25rem; + } + .organization-form-page { .el-button [class*="el-icon"] + span { - @apply ml-1; + margin-left: var(--organization-form-btn-gap); } .el-main { - @apply max-h-fit; + max-height: fit-content; } // Personal Details form .organization-details-form { & .el-form-item { - @apply mb-6; + margin-bottom: var(--organization-form-spacing); } } .identities-form .el-form-item, .custom-attributes-form .el-form-item { - @apply mb-0; + margin-bottom: 0; } .el-form .el-form-item__content, .el-form--default.el-form--label-top, .custom-attributes-form, .el-form-item__content { - @apply flex mb-0; + display: flex; + margin-bottom: 0; } } </style>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (41)
frontend/config/styles/components/button.scss(1 hunks)frontend/src/assets/scss/layout.scss(1 hunks)frontend/src/modules/activity/pages/activity-list-page.vue(1 hunks)frontend/src/modules/admin/admin.module.ts(1 hunks)frontend/src/modules/admin/admin.routes.ts(1 hunks)frontend/src/modules/admin/modules/projects/components/fragments/lf-project-column.vue(1 hunks)frontend/src/modules/admin/modules/projects/components/fragments/lf-project-count.vue(1 hunks)frontend/src/modules/admin/modules/projects/components/fragments/lf-status-pill.vue(1 hunks)frontend/src/modules/admin/modules/projects/components/lf-project-groups-dropdown.vue(1 hunks)frontend/src/modules/admin/modules/projects/components/lf-projects-dropdown.vue(1 hunks)frontend/src/modules/admin/modules/projects/components/lf-sub-projects-dropdown.vue(1 hunks)frontend/src/modules/admin/modules/projects/components/view/lf-project-groups-table.vue(2 hunks)frontend/src/modules/admin/modules/projects/components/view/lf-projects-table.vue(1 hunks)frontend/src/modules/admin/modules/projects/pages/project-groups-list.page.vue(1 hunks)frontend/src/modules/admin/modules/projects/pages/project-groups.page.vue(1 hunks)frontend/src/modules/admin/modules/projects/pages/projects.page.vue(2 hunks)frontend/src/modules/admin/pages/admin-panel.page.vue(2 hunks)frontend/src/modules/contributor/components/edit/contributor-add.vue(1 hunks)frontend/src/modules/dashboard/components/dashboard-project-group-drawer.vue(1 hunks)frontend/src/modules/index.ts(2 hunks)frontend/src/modules/lf/config/status.js(2 hunks)frontend/src/modules/lf/lf-modules.js(0 hunks)frontend/src/modules/lf/segments/components/fragments/lf-status-pill.vue(0 hunks)frontend/src/modules/lf/segments/components/lf-project-groups-dropdown.vue(0 hunks)frontend/src/modules/lf/segments/components/lf-projects-dropdown.vue(0 hunks)frontend/src/modules/lf/segments/components/view/lf-projects-table.vue(0 hunks)frontend/src/modules/organization/components/edit/organization-add.vue(1 hunks)frontend/src/modules/organization/pages/organization-form-page.vue(1 hunks)frontend/src/ui-kit/dropdown/Dropdown.vue(1 hunks)frontend/src/ui-kit/dropdown/dropdown.scss(1 hunks)frontend/src/ui-kit/index.scss(1 hunks)frontend/src/ui-kit/pill/Pill.stories.ts(2 hunks)frontend/src/ui-kit/pill/Pill.vue(1 hunks)frontend/src/ui-kit/pill/pill.scss(1 hunks)frontend/src/ui-kit/pill/types/PillColor.ts(0 hunks)frontend/src/ui-kit/pill/types/PillType.ts(1 hunks)frontend/src/ui-kit/popover/Popover.stories.ts(1 hunks)frontend/src/ui-kit/popover/Popover.vue(1 hunks)frontend/src/ui-kit/popover/popover.scss(1 hunks)frontend/src/ui-kit/popover/types/PopoverPlacement.ts(1 hunks)frontend/src/ui-kit/popover/types/PopoverTrigger.ts(1 hunks)
💤 Files with no reviewable changes (6)
- frontend/src/modules/lf/lf-modules.js
- frontend/src/ui-kit/pill/types/PillColor.ts
- frontend/src/modules/lf/segments/components/fragments/lf-status-pill.vue
- frontend/src/modules/lf/segments/components/lf-projects-dropdown.vue
- frontend/src/modules/lf/segments/components/lf-project-groups-dropdown.vue
- frontend/src/modules/lf/segments/components/view/lf-projects-table.vue
✅ Files skipped from review due to trivial changes (3)
- frontend/src/ui-kit/index.scss
- frontend/src/modules/contributor/components/edit/contributor-add.vue
- frontend/src/modules/admin/admin.module.ts
🔇 Additional comments (29)
frontend/src/ui-kit/pill/types/PillType.ts (1)
1-1: Verify the impact of type changes across the codebase.
The change from style-based types ('solid', 'bordered') to semantic types is a good improvement for maintainability. However, this is a breaking change that requires careful migration.
Let's verify the usage and potential impact:
frontend/src/assets/scss/layout.scss (1)
5-7: Verify the impact of overflow-hidden on the root element
Adding overflow-hidden to the HTML element could prevent scrolling throughout the application. This might cause issues with:
- Long content becoming inaccessible
- Modal/popover content being cut off
- Mobile responsiveness
Please ensure this change doesn't negatively impact the user experience.
✅ Verification successful
The overflow-hidden on HTML is intentional and works with the app's scroll architecture
The overflow-hidden on the HTML element is part of a deliberate scrolling architecture where:
- The HTML root is locked to prevent double scrollbars
- Scrolling is managed through dedicated components (
ScrollControllandScrollBodyControll) - The main content area (
el-main) is constrained withmax-height: 100vh - Proper scroll event handling is implemented for infinite scrolling and content overflow
This is a common pattern for single-page applications to have better control over scrolling behavior and prevent scroll issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential scroll-dependent components that might be affected
rg -l "overflow-y|overflow-x|scroll|modal|drawer|popover" --type html --type vue
Length of output: 110
Script:
#!/bin/bash
# Search for potential scroll-dependent components with broader file types
rg -l "overflow-y|overflow-x|scroll|modal|drawer|popover" --type-add 'web:*.{vue,html,js,jsx,ts,tsx}' --type web
# Also check for any CSS/SCSS files that might have scroll-related styles
rg -l "overflow|scroll|modal|drawer|popover" --type css
Length of output: 15827
Script:
#!/bin/bash
# Check the specific content of key layout and component files to understand scroll handling
cat frontend/src/assets/scss/layout.scss
cat frontend/src/ui-kit/scrollcontroll/ScrollControll.vue
cat frontend/src/ui-kit/scrollcontroll/ScrollBodyControll.vue
Length of output: 3241
frontend/src/ui-kit/popover/popover.scss (1)
11-71: 🛠️ Refactor suggestion
Add documentation and handle edge cases
The placement logic is comprehensive but could benefit from some improvements:
- Add documentation for CSS variables:
// Add at the top of the file:
/*
* CSS Variables used:
* --lf-popover-trigger-left: Left position of trigger element
* --lf-popover-trigger-right: Right position of trigger element
* --lf-popover-trigger-top: Top position of trigger element
* --lf-popover-trigger-bottom: Bottom position of trigger element
* --lf-popover-trigger-width: Width of trigger element
* --lf-popover-trigger-height: Height of trigger element
* --lf-popover-content-width: Width of popover content
* --lf-popover-content-height: Height of popover content
* --lf-popover-spacing: Spacing between trigger and content
*/- Consider adding viewport boundary detection:
The popover might overflow the viewport in certain positions. Consider implementing a JavaScript-based position adjustment system that can detect viewport boundaries and switch to an alternative placement when needed.
- Add RTL support:
If RTL support is needed, consider using logical properties:
- left: var(--lf-popover-trigger-left);
+ inset-inline-start: var(--lf-popover-trigger-left);frontend/src/ui-kit/dropdown/Dropdown.vue (2)
6-15: Verify whether clicking inside the dropdown should immediately close it.
You are calling "close" on any click within the dropdown container, which may prevent users from interacting with elements inside it. If this behavior is unintentional, consider removing the click handler or delegating closure to a specific button or action within the slot.
20-20: Confirm that 'LfPopover' is correctly registered or globally available.
Ensure that 'LfPopover' is either globally registered in your app or imported wherever you intend to use this dropdown component.
frontend/src/ui-kit/pill/Pill.vue (1)
13-19: Props API improvement looks good
The simplification of props by removing the color prop in favor of type improves the component's API. The default value of 'primary' aligns well with common UI patterns.
Let's verify the impact of this change:
✅ Verification successful
Props API change is safe and properly implemented
The change from color to type prop is well-implemented with no remaining usages of the old color prop in the codebase. The new type prop is properly typed with PillType, which includes all necessary variants ('primary', 'secondary', 'tertiary', 'success', 'warning', 'transparent'). The Storybook examples demonstrate correct usage of the new API across all supported types.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usage of the color prop
rg -g '*.{vue,ts,js}' "color.*?['\"](solid|transparent)"
Length of output: 161
Script:
#!/bin/bash
# Let's check for any usage of the Pill component with color prop
rg -g '*.{vue,ts,js}' '<Pill.*?color=' -A 2
# Also check for any imports or references to PillType
rg -g '*.{vue,ts,js}' 'PillType'
Length of output: 411
Script:
#!/bin/bash
# Let's check the content of PillType.ts to understand the type definition
cat frontend/src/ui-kit/pill/types/PillType.ts
# Let's also check the stories file to see the component usage examples
cat frontend/src/ui-kit/pill/Pill.stories.ts
Length of output: 1282
frontend/src/modules/lf/config/status.js (1)
Line range hint 3-24: Status class names are more semantic now
The change from color-based classes (green, yellow, gray) to semantic classes (success, warning, secondary) improves maintainability and accessibility.
Consider aligning color values with semantic names
While class names are now semantic, the color values still use direct color names. Consider updating these for consistency:
{
class: 'success',
- color: 'bg-green-500',
+ color: 'bg-success-500',
label: 'Active',
value: 'active',
},Let's check for similar patterns:
frontend/src/ui-kit/pill/pill.scss (2)
Line range hint 4-30: LGTM! Well-structured modifier system
The modifier system follows good practices:
- Consistent naming convention aligned with semantic meanings (success, warning)
- Clear visual hierarchy with primary/secondary/tertiary variants
- Proper use of Tailwind's color system
1-2: Consider handling overflow scenarios
The change from max-height to fixed height (h-5) might cause content overflow issues. Consider adding overflow handling or allowing height flexibility for longer content.
frontend/src/modules/index.ts (1)
13-13: Verify the module transition impact
The removal of the lf module and addition of the admin module represents a significant structural change. Please ensure all routes and components have been properly migrated.
Also applies to: 30-30
✅ Verification successful
No remaining references to lf module in active use
Based on the search results, while there are references to the lf module in the codebase, they are all within the old module directory frontend/src/modules/lf/ which is being removed. The new admin module is properly referenced in multiple files showing successful migration. No active imports or usage of the old lf module were found outside its own directory.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining references to the old lf module
echo "Checking for remaining references to lf module..."
rg -l "from ['\"]\@\/modules\/lf\/" --type vue --type ts
# Check for any components still using old paths
echo "Checking for components using old paths..."
rg -l "import.*from ['\"]\@\/modules\/lf\/segments\/" --type vue --type ts
Length of output: 573
Script:
#!/bin/bash
# Check for any remaining references to the old lf module, escaping @ properly
echo "Checking for remaining references to lf module..."
rg -l "from ['\"]@/modules/lf/" --type vue --type ts
# Check for any components still using old paths
echo "Checking for components using old paths..."
rg -l "import.*from ['\"]@/modules/lf/segments/" --type vue --type ts
# Check for any other references to lf module
echo "Checking for other lf module references..."
rg -l "/modules/lf" --type vue --type ts
# Check for admin module usage to verify migration
echo "Checking admin module usage..."
rg -l "from ['\"]@/modules/admin/" --type vue --type ts
Length of output: 713
Script:
#!/bin/bash
# Check for any remaining references to the old lf module
echo "Checking for remaining references to lf module..."
rg -l "modules/lf"
# Check for admin module usage to verify migration
echo "Checking admin module usage..."
rg -l "modules/admin"
# List all .vue and .ts files to verify file type support
echo "Listing Vue and TypeScript files..."
fd "\.(vue|ts)$"
Length of output: 69017
frontend/src/ui-kit/pill/Pill.stories.ts (2)
12-12: LGTM: Type prop default value change aligns with common UI patterns.
The change from 'default' to 'primary' as the default value follows standard UI component library patterns where 'primary' represents the main variant.
27-64: LGTM: Story renaming improves semantic meaning.
The renaming of stories from color-based names (Green, Yellow, etc.) to semantic names (Primary, Secondary, etc.) is a good improvement that:
- Makes the component's purpose clearer
- Aligns with common UI component patterns
- Makes the stories more maintainable
frontend/src/modules/admin/modules/projects/components/fragments/lf-project-column.vue (1)
30-30: LGTM: Import path updated to use absolute path.
The change from relative to absolute import path improves maintainability and aligns with the module restructuring.
frontend/src/modules/admin/modules/projects/components/lf-sub-projects-dropdown.vue (1)
21-23: LGTM: Custom UI kit components imported.
The addition of custom dropdown components from the UI kit improves consistency across the application.
frontend/src/modules/admin/admin.routes.ts (1)
7-7: LGTM! Import paths correctly updated
The component imports have been properly relocated to the new admin module structure, maintaining the existing route configuration and permission guards.
Also applies to: 11-11, 15-15
frontend/src/ui-kit/popover/Popover.stories.ts (1)
6-49: LGTM! Well-structured Storybook configuration.
The story configuration follows best practices with:
- Clear component title and autodocs tag
- Comprehensive prop documentation with descriptions and controls
- Well-defined slot documentation
frontend/src/modules/admin/modules/projects/components/lf-project-groups-dropdown.vue (1)
40-75: LGTM! Clean script setup implementation.
The component follows Vue 3 Composition API best practices with:
- Clear prop definitions
- Event emitters
- Clean separation of concerns
frontend/src/modules/admin/modules/projects/components/view/lf-project-groups-table.vue (2)
74-74: LGTM! Import path update aligns with module reorganization.
The import path update correctly reflects the new location of the dropdown component in the admin module.
21-21: Verify the impact of fixed width on status pill.
Adding w-20 class makes the status pill width fixed. Ensure this doesn't cause text overflow issues with longer status texts.
✅ Verification successful
Based on the search results, I can now provide a definitive response about the status pill width concern:
Fixed width of 5rem (w-20) is appropriate for all possible status values.
The LfStatusPill component is used to display project group statuses which can only be one of these values:
- "Active" (6 chars)
- "Formation" (9 chars)
- "Prospect" (8 chars)
- "Archived" (8 chars)
The Tailwind w-20 class provides 5rem (80px) width, which is sufficient to accommodate all these status labels without text overflow, considering:
- The longest status text is "Formation" with 9 characters
- The component uses standard font sizes
- The status labels are predefined and cannot be customized
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all possible status values to verify they fit within w-20 (5rem/80px)
rg -A 1 "status.*=.*['\"].*['\"].*" --type vue
Length of output: 80
Script:
#!/bin/bash
# Search for status-related code in all files
rg -g '!{dist,build}/*' -A 3 'status.*=.*["\'].*["\']'
# Search for the status pill component definition
fd -e vue -e js -e ts "lf-status-pill" --exec cat {}
Length of output: 116
Script:
#!/bin/bash
# Search for status-related code in all files
rg -g "!{dist,build}/*" -A 3 "status.*=.*['\"].*['\"]"
# Search for the status pill component definition
fd "lf-status-pill" -e vue -e js -e ts -x cat {}
Length of output: 35954
frontend/src/modules/admin/pages/admin-panel.page.vue (2)
37-56: LGTM! Template structure improvements
The consolidation of tab content under a single div with v-else-if conditions improves code maintainability and reduces DOM elements while maintaining the same functionality.
70-70: LGTM! Import path update
The import path update aligns with the project's restructuring efforts, moving project-related components to the admin/modules/projects directory.
frontend/src/modules/admin/modules/projects/pages/project-groups.page.vue (1)
76-79: LGTM! Consistent import path updates
The import paths have been updated to reflect the new project structure, moving project-related components to a dedicated projects module. This improves code organization and maintainability.
frontend/src/modules/activity/pages/activity-list-page.vue (1)
90-90: LGTM! Consistent import path update
The import path update aligns with the project's restructuring efforts.
frontend/src/modules/admin/modules/projects/pages/project-groups-list.page.vue (1)
125-125: LGTM: Import path updated correctly
The import path has been updated to reflect the new module structure.
frontend/src/modules/organization/components/edit/organization-add.vue (1)
120-120: LGTM: Import path updated correctly
The import path has been updated to reflect the new module structure.
frontend/src/modules/dashboard/components/dashboard-project-group-drawer.vue (1)
158-158: LGTM: Import path updated correctly.
The import path for AppLfSearchInput has been updated to reflect the new module structure.
frontend/config/styles/components/button.scss (1)
139-139: LGTM: Improved text contrast for better accessibility.
The text colors for the secondary ghost light button variant have been updated to use darker shades, which improves readability and accessibility:
- Normal state: Changed from gray-400 to gray-500
- Hover state: Changed from gray-500 to gray-600
- Active state: Changed from gray-500 to gray-600
Also applies to: 144-144, 148-148
frontend/src/modules/admin/modules/projects/pages/projects.page.vue (1)
156-160: LGTM: Import paths updated consistently.
All component imports have been correctly updated to reflect the new module structure, moving from lf/segments to admin/modules/projects. The changes are consistent with the broader refactoring effort seen across the PR.
Also applies to: 169-171
frontend/src/modules/organization/pages/organization-form-page.vue (1)
166-166: Verify the component's new location and its impact.
The import path change aligns with the module restructuring effort. However, we should ensure this change is consistent across the codebase.
✅ Verification successful
Let me check the imports in the other files to ensure consistency.
Import path change is consistent across the codebase
The component exists at the new location frontend/src/modules/admin/modules/projects/components/lf-sub-projects-list-dropdown.vue, and all files that use this component are already using the new import path @/modules/admin/modules/projects/components/lf-sub-projects-list-dropdown.vue. The change is properly aligned with the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the old import path is still being used elsewhere
rg -l "@/modules/lf/segments/components/lf-sub-projects-list-dropdown.vue"
# Verify the new component location exists
fd "lf-sub-projects-list-dropdown.vue" "frontend/src/modules/admin/modules/projects/components"
# Find all other imports of this component to ensure consistency
rg -l "lf-sub-projects-list-dropdown.vue"
Length of output: 592
Script:
#!/bin/bash
# Check import statements in files that use the component
rg -A 1 -B 1 "lf-sub-projects-list-dropdown" frontend/src/modules/organization/components/edit/organization-add.vue frontend/src/modules/contributor/components/edit/contributor-add.vue frontend/src/modules/admin/modules/projects/components/lf-sub-projects-list-modal.vue
Length of output: 2451
| &__content { | ||
| @apply absolute z-50 w-max transition-all overflow-visible; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize transitions and z-index management
A few suggestions to improve performance and maintainability:
- Replace
transition-allwith specific transitions to improve performance:
- @apply absolute z-50 w-max transition-all overflow-visible;
+ @apply absolute z-50 w-max overflow-visible;
+ transition: transform 0.2s ease-in-out, opacity 0.2s ease-in-out;- Consider using CSS custom properties for z-index to maintain a consistent stacking context across the application:
- @apply absolute z-50 w-max transition-all overflow-visible;
+ @apply absolute w-max transition-all overflow-visible;
+ z-index: var(--lf-z-index-popover, 50);Committable suggestion skipped: line range outside the PR's diff.
| <lf-badge type="secondary" class="!flex items-center gap-1 w-full !rounded-full !px-2.5"> | ||
| <lf-icon-old name="stack-line" /> | ||
| {{ pluralize('project', props.count, true) }} | ||
| </lf-pill> | ||
| </lf-badge> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review utility class overrides
The use of multiple ! important flags in utility classes suggests potential style conflicts:
class="!flex items-center gap-1 w-full !rounded-full !px-2.5"Consider addressing the root cause of these style conflicts instead of using important flags.
| import pluralize from 'pluralize'; | ||
| import LfPill from '@/ui-kit/pill/Pill.vue'; | ||
| import LfIconOld from '@/ui-kit/icon/IconOld.vue'; | ||
| import LfBadge from '@/ui-kit/badge/Badge.vue'; | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
One remaining LfPill component needs to be migrated
The UI modernization effort to replace LfPill with LfBadge is incomplete. There is still one component using the old LfPill:
frontend/src/modules/admin/modules/projects/components/fragments/lf-status-pill.vue
This component should also be updated to use LfBadge for consistency with the UI modernization efforts.
🔗 Analysis chain
Component imports and replacement look good
The replacement of LfPill with LfBadge aligns with the UI modernization efforts seen across the codebase.
Let's verify no orphaned pill components remain:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for remaining LfPill usage
rg -g '*.vue' "import.*LfPill.*from"
Length of output: 167
Script:
#!/bin/bash
# Let's check if LfPill is still being used in lf-status-pill.vue
cat frontend/src/modules/admin/modules/projects/components/fragments/lf-status-pill.vue
Length of output: 753
| const statusDisplay: Record<string, any> = { | ||
| active: { | ||
| type: 'success', | ||
| label: 'Active', | ||
| }, | ||
| formation: { | ||
| type: 'primary', | ||
| label: 'Formation', | ||
| }, | ||
| prospect: { | ||
| type: 'warning', | ||
| label: 'Prospect', | ||
| }, | ||
| archived: { | ||
| type: 'secondary', | ||
| label: 'Archived', | ||
| }, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety of status mapping
The current Record<string, any> type is too permissive. Consider creating proper interfaces:
interface StatusConfig {
type: 'success' | 'primary' | 'warning' | 'secondary';
label: string;
}
const statusDisplay: Record<string, StatusConfig> = {
active: {
type: 'success',
label: 'Active',
},
// ... rest of the mapping
};| v-if="(hasPermission(LfPermission.subProjectCreate) && hasAccessToSegmentId(id)) | ||
| || (hasPermission(LfPermission.projectEdit) && hasAccessToSegmentId(id))" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify permission checks
The permission check logic is duplicated and could be simplified into a computed property.
+ const canShowDropdown = computed(() => {
+ return (hasPermission(LfPermission.subProjectCreate) || hasPermission(LfPermission.projectEdit))
+ && hasAccessToSegmentId(id);
+ });
- v-if="(hasPermission(LfPermission.subProjectCreate) && hasAccessToSegmentId(id))
- || (hasPermission(LfPermission.projectEdit) && hasAccessToSegmentId(id))"
+ v-if="canShowDropdown"Committable suggestion skipped: line range outside the PR's diff.
| <article | ||
| v-for="subproject of props.project.subprojects" | ||
| :key="subproject.id" | ||
| class="border-t border-gray-200 px-4 py-3 flex items-center justify-between" | ||
| > | ||
| <div class="flex items-center gap-4"> | ||
| <lf-project-status-pill :status="subproject.status" class="w-20" /> | ||
| <p class="text-medium"> | ||
| {{ subproject.name }} | ||
| </p> | ||
| <app-lf-project-integration-column | ||
| :segment-id="subproject.id" | ||
| :integrations="subproject.integrations" | ||
| :progress="props.progress" | ||
| :progress-error="progressError" | ||
| /> | ||
| </div> | ||
| <div class="flex items-center gap-2"> | ||
| <router-link | ||
| :to="{ | ||
| name: 'integration', | ||
| params: { | ||
| id: subproject.id, | ||
| grandparentId: route.params.id, | ||
| }, | ||
| }" | ||
| > | ||
| <lf-button type="primary-ghost"> | ||
| <lf-icon name="grid-round-2-plus" :size="16" type="regular" /> | ||
| Integrations | ||
| </lf-button> | ||
| </router-link> | ||
| <app-lf-sub-projects-dropdown | ||
| :id="subproject.id" | ||
| @on-edit-sub-project="emit('onEditSubProject', subproject.id)" | ||
| /> | ||
| </div> | ||
| </article> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add key accessibility attributes to interactive elements
The subprojects section contains interactive elements that should have proper accessibility attributes.
<article
v-for="subproject of props.project.subprojects"
:key="subproject.id"
class="border-t border-gray-200 px-4 py-3 flex items-center justify-between"
>
<div class="flex items-center gap-4">
- <lf-project-status-pill :status="subproject.status" class="w-20" />
+ <lf-project-status-pill
+ :status="subproject.status"
+ class="w-20"
+ :aria-label="`Status: ${subproject.status}`"
+ />
<p class="text-medium">
{{ subproject.name }}
</p>
</div>
<div class="flex items-center gap-2">
<router-link
:to="{
name: 'integration',
params: {
id: subproject.id,
grandparentId: route.params.id,
},
}"
+ :aria-label="`View integrations for ${subproject.name}`"
>Committable suggestion skipped: line range outside the PR's diff.
Changes proposed ✍️
What
copilot:summary
copilot:poem
Why
How
copilot:walkthrough
Checklist ✅
Feature,Improvement, orBug.Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Style
Documentation