UISACQCOMP-168: extend <Donors /> component props
#730
Conversation
Jest Unit Test Statistics 1 files ±0 146 suites ±0 4m 5s ⏱️ +6s Results for commit 6344233. ± Comparison against base commit f28d3b2. This pull request removes 1 and adds 1 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
alisher-epam
force-pushed
the
UISACQCOMP-168
branch
from
1d8373c to
5da4eda
Compare
lib/Donors/DonorsContainer.js
Outdated
| @@ -79,7 +85,8 @@ export function DonorsContainer({ | |||
| onAddDonors={onAddDonors} | |||
| name={id} | |||
| searchLabel={searchLabel} | |||
| visibleColumns={visibleColumns} | |||
| visibleColumns={pluginVisibleColumnsProp} | |||
There was a problem hiding this comment.
do you really need pluginVisibleColumnsProp prop in this component? https://github.com/folio-org/stripes-acq-components/pull/730/files#diff-acbdf62e7810796097a7fc03795e89233de44db880d57ec20583f02a44b8060cR65
There was a problem hiding this comment.
Yes, I need it. Because Donors form visible columns include unassignDonor column name like below
There was a problem hiding this comment.
I mean <DonorsLookup> has the default visibleColumns prop with the same value you are provided as pluginVisibleColumnsProp. You don't have to pass visibleColumns value to the <DonorsLookup> here at all, right?
lib/Donors/Donors.js
Outdated
| @@ -12,10 +13,19 @@ import { defaultColumnMapping } from './constants'; | |||
| import { DonorsContainer } from './DonorsContainer'; | |||
| import { useFetchDonors } from './hooks'; | |||
|
|
|||
| export function Donors({ name, donorOrganizationIds, ...rest }) { | |||
| export function Donors({ name, donorOrganizationIds, onChange, ...rest }) { | |||
| const [donorIds, setDonorIds] = useState(donorOrganizationIds); | |||
| const { donors, isLoading } = useFetchDonors(donorIds); | |||
There was a problem hiding this comment.
Could you please apply some changes to improve the UX (keep prev changes in the MCL and display loading indicator inside the list):
// useFetchDonors.js
export const useFetchDonors = (donorOrganizationIds = DEFAULT_DATA, options = {}) => {
..
{
enabled: Boolean(donorOrganizationIds.length)
...options,
},
..
);
// ------
// Donors.js
..
const { donors, isFetching } = useFetchDonors(donorIds, { keepPreviousData: true });
..
// Remove this
if (isLoading) {
return <Loading />;
}
..
<FieldArray
..
component={DonorsContainer}
loading={isFetching}
..
/>
There was a problem hiding this comment.
This is a very nice improvement. Thank you for your advice.
|
Kudos, SonarCloud Quality Gate passed! |
Purpose
UISACQCOMP-168 -
Pass
onChangeandonRemoveprops for theDonorscomponent in order to gain more control for complex use cases. The issue with the previous implementation was adding and removing the donors' list, controlled by the component.The proposed solution helps to pass donorOrganizationIds along with the mentioned props above in order to manage the list programmatically(conditionally).
Related issues - UIOR-1148
Approach
Screen.Recording.2023-11-15.at.4.22.16.PM.mov
Pre-Merge Checklist
Before merging this PR, please go through the following list and take appropriate actions.
If there are breaking changes, please STOP and consider the following:
Ideally all of the PRs involved in breaking changes would be merged in the same day to avoid breaking the folio-testing environment. Communication is paramount if that is to be achieved, especially as the number of intermodule and inter-team dependencies increase.
While it's helpful for reviewers to help identify potential problems, ensuring that it's safe to merge is ultimately the responsibility of the PR assignee.