Skip to content

Enhancement/financial request menu UI#19

Open
Herlin6 wants to merge 4 commits into
developmentfrom
enhancement/financial-request-menu-ui
Open

Enhancement/financial request menu UI#19
Herlin6 wants to merge 4 commits into
developmentfrom
enhancement/financial-request-menu-ui

Conversation

@Herlin6
Copy link
Copy Markdown

@Herlin6 Herlin6 commented Aug 3, 2024

No description provided.

Herlin6 added 3 commits July 28, 2024 18:34
- edit search bar and SortBy filter
- add trigger for a modal popup in the search bar
- add SortBy button in table header
- add clear filter button
@Herlin6 Herlin6 requested a review from fanesz August 3, 2024 09:50
@Herlin6 Herlin6 self-assigned this Aug 3, 2024
@Herlin6 Herlin6 linked an issue Aug 3, 2024 that may be closed by this pull request
6 tasks
Copy link
Copy Markdown
Contributor

@fanesz fanesz left a comment

Choose a reason for hiding this comment

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

komponen searchbar jangan di otak atik, itu sudah bener, delay 0.5s bakal auto apply, itulah fitur debouncer. kalo mau ditambahin fitur skip debounce on enter baru boleh.

header yg punya fitur sort blm dibikin pointer cursornya

fitur clear filter masih banyak bug

fitur sortby berdasarkan ascending/descending hilang...

Comment on lines +27 to +30
useEffect(() => {
setTempValue(value);
}, [value]);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

kan value sudah di passing lewat onChange(), ini untuk??

Comment on lines +35 to +40
height: "1.2rem"
height: "0.9rem"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

knp di pendekin?

Comment on lines +32 to +42
const filterSortBy = filters.find((filter) => filter.key === "sort");
const { handleClearSortBy } = useFinancialFilters();
const [searchValue, setSearchValue] = useState<string>("");
const handleChangeSearchBar = useCallback((value: string) => {
setSearchValue(value);
handleChangeSearch(value);
}, []);
const handleClear = useCallback(() => {
setSearchValue("");
handleClearSortBy();
}, [handleClearSortBy]);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

searchValue kan sudah di handling di component searchbar, disitu pake debouncer, jadi pas input, 0.5 detik, auto enter.
jangan pake usestate/usecallback, tapi seluruh ini di bikin function, trus pindahin ke custom hooks useFinancialFilters.
yang diclear itu state dari context, jadi useState itu unnecessary di design pattern ini.

untuk clear jg masih ngebug, misal filter by status, kadang statusnya tetep approved.

Comment on lines +114 to +133
export function StatusIcon(status: string): JSX.Element {
let icon;
const commonIconStyle = {
fontSize: "medium",
opacity: 0.5
};
if (status === "Rejected") {
icon = <ErrorOutlineIcon color="error" sx={commonIconStyle} />;
} else if (status === "Pending") {
icon = <WarningAmberIcon color="warning" sx={commonIconStyle} />;
} else {
icon = <CheckCircleOutlineIcon color="success" sx={commonIconStyle} />;
}

return (
<>
{icon} {status}
</>
);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

???????????????????????????????????????

kalo emang butuh komponen terpisah, bikin lah di folder partials, context itu cuma untuk simpen STATE / DATA

Comment on lines +16 to +21
{ label: "id", width: "w-20", sortBy: "" },
{ label: "username", width: "w-44", sortBy: "" },
{ label: "amount", width: "w-32", sortBy: "" },
{ label: "status", width: "w-28", sortBy: sortBy(2) },
{ label: "note", width: "w-52", sortBy: "" },
{ label: "date", width: "w-24", sortBy: sortBy(1) }
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

sort ttp harus di context, maenin function untuk set yang di sort, bukan disimpen jadi variable object

Comment on lines +4 to +5
// TableFilter,
// TableFilterItem,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ilangin comment

<div className="-mt-1">
<Tooltip title={currentFinreq.created_at} arrow>
<Tooltip
title={moment(currentFinreq.created_at).format("dddd, hh:mm A")}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

tampilin detik

Comment thread src/renderer/src/common/types/index.ts Outdated
key: string;
label: string;
options: CommonOptions[];
value: Array<string>;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
value: Array<string>;

di context, sudah ada variable filters, value ditampung disitu, bukan useState ini

Comment on lines +139 to +142
const handleClearSortBy = () => {
handleChangeSortBy("order_by", ["asc"]);
handleChangeSortBy("status", []);
};
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ini yg di clear itu context

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

setState
...prevState
filters: {
... copas filters default yg skrg di context
}

Comment on lines +11 to +21
return (
<>
<SwapVertIcon
sx={{
fontSize: "medium"
}}
onClick={handleSortItem}
/>
</>
);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
return (
<>
<SwapVertIcon
sx={{
fontSize: "medium"
}}
onClick={handleSortItem}
/>
</>
);
}
return (
<SwapVertIcon
sx={{
fontSize: "medium"
}}
onClick={handleSortItem}
/>
);
}

uneccessary

- add StatusIcon function in partials
- remove useless type declaration
- edit icon in financial request status
- add seconds to datetime format in Created At tooltip
- edit handleChangeSortBy
 and handleClearSortBy functions in useFinancialRequest
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.

Enhancement/Financial Request Menu UI

2 participants