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

4410-feat(front): Implement Confirmation Prompt for Multiple Record Deletion #4514

Merged
merged 17 commits into from
Mar 29, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { useRecoilValue } from 'recoil';
import { useRecoilState, useRecoilValue } from 'recoil';

import { useRecordBoardStates } from '@/object-record/record-board/hooks/internal/useRecordBoardStates';
import { selectedRecordsComponentState } from '@/object-record/record-table/states/selectedRecordsComponentState';
import { ActionBar } from '@/ui/navigation/action-bar/components/ActionBar';

type RecordBoardActionBarProps = {
Expand All @@ -14,7 +15,13 @@ export const RecordBoardActionBar = ({

const selectedRecordIds = useRecoilValue(getSelectedRecordIdsSelector());

if (!selectedRecordIds.length) {
const [selectedRecords, setSelectedRecords] = useRecoilState(
selectedRecordsComponentState(),
);

setSelectedRecords(selectedRecordIds.length);

if (!selectedRecords) {
return null;
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { useRecoilValue } from 'recoil';
import { useRecoilState, useRecoilValue } from 'recoil';

import { useRecordTableStates } from '@/object-record/record-table/hooks/internal/useRecordTableStates';
import { selectedRecordsComponentState } from '@/object-record/record-table/states/selectedRecordsComponentState';
import { ActionBar } from '@/ui/navigation/action-bar/components/ActionBar';

export const RecordTableActionBar = ({
Expand All @@ -12,7 +13,13 @@ export const RecordTableActionBar = ({

const selectedRowIds = useRecoilValue(getSelectedRowIdsSelector());

if (!selectedRowIds.length) {
const [selectedRecords, setSelectedRecords] = useRecoilState(
selectedRecordsComponentState(),
);

setSelectedRecords(selectedRowIds.length);

if (!selectedRecords) {
return null;
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import { createState } from '@/ui/utilities/state/utils/createState';

export const selectedRecordsComponentState = createState<number>({
key: 'selectedRecordsComponentState',
defaultValue: 0,
});
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,12 @@ const StyledContainerActionBar = styled.div`
display: flex;
height: 48px;

left: 50%;
left: 40%;
Copy link
Contributor

Choose a reason for hiding this comment

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

I was unable to find the design specifications for this in figma. Where did this come from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Left: 50% and transform: translateX(-50%); centers an absolute positioned element. But after adding the confirmation modal, the size of the modal reduced to the size of the actionbar. To address this issue, I removed transform: translateX(-50%); and decreased the percentage of Left to 40% to center the element on the screen.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ohk maybe we can proceed as there's no specific design regarding this

Copy link
Contributor

Choose a reason for hiding this comment

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

Here 50% was okay as ig you are viewing it as centre of screen but it's centre of record table. and z-index, transform change is not required.

padding-left: ${({ theme }) => theme.spacing(2)};
padding-right: ${({ theme }) => theme.spacing(2)};
position: absolute;
top: auto;

transform: translateX(-50%);
z-index: 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

This too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had increased the value of z-index to ensure the modal appears above all other elements.

z-index: 20;
`;

export const ActionBar = () => {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,15 @@
import { useState } from 'react';
import { useTheme } from '@emotion/react';
import styled from '@emotion/styled';
import { useRecoilState } from 'recoil';
import { MenuItem } from 'tsup.ui.index';

import { selectedRecordsComponentState } from '@/object-record/record-table/states/selectedRecordsComponentState';
import { IconChevronDown } from '@/ui/display/icon';
import { Dropdown } from '@/ui/layout/dropdown/components/Dropdown';
import { DropdownMenuItemsContainer } from '@/ui/layout/dropdown/components/DropdownMenuItemsContainer';
import { useDropdown } from '@/ui/layout/dropdown/hooks/useDropdown';
import { ConfirmationModal } from '@/ui/layout/modal/components/ConfirmationModal';
import { ActionBarEntry } from '@/ui/navigation/action-bar/types/ActionBarEntry';
import { MenuItemAccent } from '@/ui/navigation/menu-item/types/MenuItemAccent';

Expand Down Expand Up @@ -44,6 +48,10 @@ export const ActionBarItem = ({ item }: ActionBarItemProps) => {
const theme = useTheme();
const dropdownId = `action-bar-item-${item.label}`;
const { toggleDropdown, closeDropdown } = useDropdown(dropdownId);
const [isDeleteRecordsModalOpen, setIsDeleteRecordsModalOpen] =
useState(false);
const [selectedRecords] = useRecoilState(selectedRecordsComponentState());

return (
<>
{Array.isArray(item.subActions) ? (
Expand Down Expand Up @@ -80,13 +88,33 @@ export const ActionBarItem = ({ item }: ActionBarItemProps) => {
}
/>
) : (
<StyledButton
accent={item.accent ?? 'default'}
onClick={() => item.onClick?.()}
>
{item.Icon && <item.Icon size={theme.icon.size.md} />}
<StyledButtonLabel>{item.label}</StyledButtonLabel>
</StyledButton>
<>
<StyledButton
accent={item.accent ?? 'default'}
onClick={() =>
item.label === 'Delete'
? setIsDeleteRecordsModalOpen(true)
: item.onClick?.()
}
>
{item.Icon && <item.Icon size={theme.icon.size.md} />}
<StyledButtonLabel>{item.label}</StyledButtonLabel>
</StyledButton>
{item.label === 'Delete' && (
<ConfirmationModal
isOpen={isDeleteRecordsModalOpen}
setIsOpen={setIsDeleteRecordsModalOpen}
title={`Delete ${selectedRecords} ${
selectedRecords === 1 ? `record` : 'records'
}`}
subtitle={`This action cannot be undone. This will permanently delete ${
selectedRecords === 1 ? 'this record' : 'these records'
}`}
onConfirmClick={() => item.onClick?.()}
deleteButtonText="Delete Records"
/>
)}
</>
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem to be a good approach as the base action for Delete is already implemented and conditional approach isn't good. We can keep the ActionBarItem code as it is. Instead please make changes to useRecordActionBar

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tbh, i had doubts regarding my implementation. So, thanks for the review. I have made the necessary changes. Is this what you meant?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes basically I was suggesting reusability and not hard coded.

)}
</>
);
Expand Down
Loading