Skip to content

Conversation

@yaroslav8765
Copy link
Contributor

No description provided.

@yaroslav8765 yaroslav8765 requested a review from Copilot October 27, 2025 12:37
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a three-dots menu for each record in the list view, providing better organization of action buttons. The implementation adds flexibility to display actions either as quick icons or within a collapsible menu, improving UI scalability when multiple actions are available.

Key Changes:

  • Added new customActionIconsThreeDotsMenuItems configuration option for custom menu items
  • Introduced listQuickIcon and listThreeDotsMenu properties to control action visibility
  • Created new ListActionsThreeDots component with smart positioning logic
  • Added moveBaseActionsOutOfThreeDotsMenu option to control base action placement

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
adminforth/types/Common.ts Added customActionIconsThreeDotsMenuItems type definition for list page injections
adminforth/types/Back.ts Replaced list property with listQuickIcon and listThreeDotsMenu for granular action placement control; added moveBaseActionsOutOfThreeDotsMenu option
adminforth/spa/src/views/ListView.vue Added conditional rendering guard and props for custom three-dots menu items; includes debug console.log
adminforth/spa/src/components/ResourceListTableVirtual.vue Integrated three-dots menu component with conditional rendering of base actions and custom actions
adminforth/spa/src/components/ResourceListTable.vue Integrated three-dots menu component (same changes as virtual table)
adminforth/spa/src/components/ListActionsThreeDots.vue New component implementing three-dots dropdown menu with smart positioning and teleport
adminforth/modules/configValidator.ts Updated validation to support new action visibility properties and page injection keys

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

await init();
initThreeDotsDropdown();
initInProcess = false;
console.log('ListView initialized:', coreStore.resourceOptions?.pageInjections?.list?.customActionIconsThreeDotsMenuItems);
Copy link

Copilot AI Oct 27, 2025

Choose a reason for hiding this comment

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

Debug console.log statement should be removed before merging to production. This appears to be leftover debugging code.

Suggested change
console.log('ListView initialized:', coreStore.resourceOptions?.pageInjections?.list?.customActionIconsThreeDotsMenuItems);
// ListView initialized (debug log removed for production)

Copilot uses AI. Check for mistakes.
Delete item
</button>
</template>
<div v-for="action in resourceOptions.actions.filter(a => a.showIn?.listThreeDotsMenu)" :key="action.id" >
Copy link

Copilot AI Oct 27, 2025

Choose a reason for hiding this comment

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

Potential runtime error if resourceOptions.actions is undefined or null. The code attempts to call .filter() without first checking if resourceOptions.actions exists.

Suggested change
<div v-for="action in resourceOptions.actions.filter(a => a.showIn?.listThreeDotsMenu)" :key="action.id" >
<div v-for="action in (resourceOptions.actions ?? []).filter(a => a.showIn?.listThreeDotsMenu)" :key="action.id" >

Copilot uses AI. Check for mistakes.
const gap = 8;
menuStyles.value = {
position: 'fixed',
top: `${Math.round(rect.bottom )}px`,
Copy link

Copilot AI Oct 27, 2025

Choose a reason for hiding this comment

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

Extra space before closing parenthesis in rect.bottom ).

Suggested change
top: `${Math.round(rect.bottom )}px`,
top: `${Math.round(rect.bottom)}px`,

Copilot uses AI. Check for mistakes.
*/
export interface ResourceOptionsInput extends Omit<NonNullable<AdminForthResourceInputCommon['options']>, 'allowedActions' | 'bulkActions'> {

moveBaseActionsOutOfThreeDotsMenu?: boolean,
Copy link
Contributor

Choose a reason for hiding this comment

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

@yaroslav8765 could we please update this type to ('show'| 'edit'| 'delete')[] , I think we need to give more flexibility here, and rename to moveBaseActionsOutOfThreeDotsMenu -> baseActionsAsQuickIcons,

Copy link
Contributor

Choose a reason for hiding this comment

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

Also we should add a section called "Base actions as Icons" to standard pages tuning docs

<div
ref="triggerRef"
class="border border-gray-300 dark:border-gray-700 dark:border-opacity-0 border-opacity-0 hover:border-opacity-100 dark:hover:border-opacity-100 rounded-md hover:bg-gray-100 dark:hover:bg-gray-800 cursor-pointer"
@click="toggleMenu"
Copy link
Contributor

Choose a reason for hiding this comment

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

@yaroslav8765 can you open menu on @hover also (and close on @blur

</Tooltip>
</template>
<ListActionsThreeDots
v-if="resource.options?.actions?.some(a => a.showIn?.listThreeDotsMenu) || (props.customActionIconsThreeDotsMenuItems && props.customActionIconsThreeDotsMenuItems.length > 0) || resource.options.moveBaseActionsOutOfThreeDotsMenu !== true"
Copy link
Contributor

Choose a reason for hiding this comment

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

JIC this will be reworked after making moveBaseActionsOutOfThreeDotsMenu change,
after rework it should first filter out only threeDotsmenu items and then get their length, possible to do in one-line also though

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants