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

UIPFU-91 - React v19: refactor away from default props for functional components #285

Merged
merged 19 commits into from
Jan 15, 2025
Merged
Show file tree
Hide file tree
Changes from all 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
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
# Change history for ui-plugin-find-user

## In progress

* React v19: refactor away from default props for functional components. Refs UIPFU-91.

## [7.2.0] (https://github.com/folio-org/ui-plugin-find-user/tree/v7.2.0) (2024-09-05)
[Full Changelog](https://github.com/folio-org/ui-plugin-find-user/compare/v7.1.2...v7.2.0)

Expand Down
73 changes: 32 additions & 41 deletions src/Filters.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,50 +5,41 @@ import {
FilterGroups,
} from '@folio/stripes/components';

export default class Filters extends React.Component {
static propTypes = {
activeFilters: PropTypes.object,
onChangeHandlers: PropTypes.object.isRequired,
config: PropTypes.arrayOf(PropTypes.object),
resultOffset: PropTypes.shape({
replace: PropTypes.func.isRequired,
}),
};

static defaultProps = {
activeFilters: {},
}

handleFilterChange = e => {
const {
resultOffset,
onChangeHandlers,
} = this.props;

const Filters = ({
activeFilters,
onChangeHandlers,
config,
resultOffset,
}) => {
Comment on lines +8 to +13

Choose a reason for hiding this comment

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

Class components will continue to support defaultProps since there is no ES6 alternative.

Moving class components to functional ones seems redundant doc.

React v19: refactor away from default props for functional components

Is there any reason to touch class components?

Copy link
Contributor Author

@Terala-Priyanka Terala-Priyanka Dec 30, 2024

Choose a reason for hiding this comment

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

no specific reason. As we have been following the standard of creating only functional components in new developments and also refactoring the components from class based to functional based as and when there is an opportunity, I have taken this opportunity for the refactor.

const { clearGroup } = onChangeHandlers;

const handleFilterChange = e => {
if (resultOffset) {
resultOffset.replace(0);
}

onChangeHandlers.checkbox(e);
}

render() {
const {
activeFilters,
onChangeHandlers: { clearGroup },
config,
} = this.props;

const groupFilters = {};
activeFilters.string.split(',').forEach(m => { groupFilters[m] = true; });
};

return (
<FilterGroups
config={config}
filters={groupFilters}
onChangeFilter={this.handleFilterChange}
onClearFilter={clearGroup}
/>
);
}
}
const groupFilters = {};
activeFilters.string.split(',').forEach(m => { groupFilters[m] = true; });

return (
<FilterGroups
config={config}
filters={groupFilters}
onChangeFilter={handleFilterChange}
onClearFilter={clearGroup}
/>
);
};

Filters.propTypes = {
activeFilters: PropTypes.object,
onChangeHandlers: PropTypes.object.isRequired,
config: PropTypes.arrayOf(PropTypes.object),
resultOffset: PropTypes.shape({
replace: PropTypes.func.isRequired,
}),
};
export default Filters;
6 changes: 4 additions & 2 deletions src/Filters.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,10 @@ const props = {
checkbox: jest.fn(),
},
activeFilters: {
state: {},
string: '',
state: {
active : ['active'],
},
string: 'active.active',
},
resultOffset: {
replace: jest.fn(),
Expand Down
179 changes: 85 additions & 94 deletions src/PluginFindUser.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, { Component } from 'react';
import { useState, useRef, useEffect } from 'react';
import PropTypes from 'prop-types';
import _omit from 'lodash/omit';
import className from 'classnames';
Expand All @@ -11,106 +11,97 @@ import UserSearchModal from './UserSearchModal';

import css from './UserSearch.css';

class PluginFindUser extends Component {
constructor(props) {
super(props);

this.state = {
openModal: false,
};

this.openModal = this.openModal.bind(this);
this.closeModal = this.closeModal.bind(this);
this.modalTrigger = React.createRef();
this.modalRef = React.createRef();
}

getStyle() {
const { marginBottom0, marginTop0 } = this.props;
return className(
css.searchControl,
{ [css.marginBottom0]: marginBottom0 },
{ [css.marginTop0]: marginTop0 },
);
}

openModal() {
this.setState({
openModal: true,
});
}

closeModal() {
const {
afterClose
} = this.props;

this.setState({
openModal: false,
}, () => {
const PluginFindUser = (props) => {
const {
afterClose,
id = 'clickable-plugin-find-user',
searchLabel,
searchButtonStyle = 'primary noRightRadius',
marginBottom0,
marginTop0,
// eslint-disable-next-line no-unused-vars
onModalClose,
renderTrigger,
// eslint-disable-next-line no-unused-vars
dataKey = 'find_patron',
// eslint-disable-next-line no-unused-vars
initialSelectedUsers,
} = props;
Comment on lines +22 to +28

Choose a reason for hiding this comment

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

Why are unused variables specified?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are being passed to child components. Also they need default props to be mentioned.


const [isModalOpen, setIsModalOpen] = useState(false);
const modalTrigger = useRef();
const modalRef = useRef();
const prevOpenModalRef = useRef(); // Reference to track the previous value of openModal

// don't inadvertently pass in other resources which could result in resource confusion.
const isolatedProps = _omit(props, ['parentResources', 'resources', 'mutator', 'parentMutator']);

useEffect(() => {
if (prevOpenModalRef.current && !isModalOpen) {
if (afterClose) {
afterClose();
}

if (this.modalRef.current && this.modalTrigger.current) {
if (contains(this.modalRef.current, document.activeElement)) {
this.modalTrigger.current.focus();
if (modalRef.current && modalTrigger.current) {
if (contains(modalRef.current, document.activeElement)) {
modalTrigger.current.focus();
}
}
});
}

renderTriggerButton() {
const {
renderTrigger,
} = this.props;

return renderTrigger({
buttonRef: this.modalTrigger,
onClick: this.openModal,
});
}

render() {
const { id, marginBottom0, renderTrigger, searchButtonStyle, searchLabel } = this.props;
// don't inadvertently pass in other resources which could result in resource confusion.
const isolatedProps = _omit(this.props, ['parentResources', 'resources', 'mutator', 'parentMutator']);
}
}, [isModalOpen, afterClose]);

Choose a reason for hiding this comment

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

I think adding afterClose function to deps is unsafe, as it now always needs to be memorized using useCallback to avoid triggering useEffect multiple times. The previous implementation didn't require this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Earlier Class based approach had a provision to make callbacks after setting while setState. Functional components do not work this way, right?
Also afterClose is a prop and called only when the modal is closed and only based on some conditions, I think it is fine here.

return (
<div className={this.getStyle()} data-test-plugin-find-user>
{renderTrigger ?
this.renderTriggerButton() :
<FormattedMessage id="ui-plugin-find-user.searchButton.title">
{ariaLabel => (
<Button
id={id}
key="searchButton"
buttonStyle={searchButtonStyle}
aria-label={ariaLabel}
onClick={this.openModal}
buttonRef={this.modalTrigger}
marginBottom0={marginBottom0}
data-testid="searchButton"
>
{searchLabel || <Icon icon="search" color="#fff" />}
</Button>
)}
</FormattedMessage>}
<UserSearchModal
openWhen={this.state.openModal}
closeCB={this.closeModal}
modalRef={this.modalRef}
{...isolatedProps}
/>
</div>
);
}
}

PluginFindUser.defaultProps = {
id: 'clickable-plugin-find-user',
searchButtonStyle: 'primary noRightRadius',
dataKey: 'find_patron',
const getStyle = () => (
className(
css.searchControl,
{ [css.marginBottom0]: marginBottom0 },
{ [css.marginTop0]: marginTop0 },
)
);

const openModal = () => {
setIsModalOpen(true);
};

const closeModal = () => {
prevOpenModalRef.current = isModalOpen;
setIsModalOpen(false);
};

const renderTriggerButton = () => (
renderTrigger({
buttonRef: modalTrigger,
onClick: openModal,
})
);

return (
<div className={getStyle()} data-test-plugin-find-user>
{renderTrigger ?
renderTriggerButton() :
<FormattedMessage id="ui-plugin-find-user.searchButton.title">
{ariaLabel => (
<Button
id={id}
key="searchButton"
buttonStyle={searchButtonStyle}
aria-label={ariaLabel}
onClick={openModal}
buttonRef={modalTrigger}
marginBottom0={marginBottom0}
data-testid="searchButton"
>
{searchLabel || <Icon icon="search" color="#fff" />}
</Button>
)}
</FormattedMessage>}
<UserSearchModal
openWhen={isModalOpen}
closeCB={closeModal}
modalRef={modalRef}
{...isolatedProps}
/>
</div>
);
};

PluginFindUser.propTypes = {
Expand Down
21 changes: 6 additions & 15 deletions src/PluginFindUser.test.js
Original file line number Diff line number Diff line change
@@ -1,22 +1,13 @@
import { screen } from '@folio/jest-config-stripes/testing-library/react';
import { screen, waitFor } from '@folio/jest-config-stripes/testing-library/react';
import userEvent from '@folio/jest-config-stripes/testing-library/user-event';

import renderWithRouter from 'helpers/renderWithRouter';

import PluginFindUser from './PluginFindUser';

jest.unmock('@folio/stripes/components');
jest.mock('./UserSearchModal', () => ({ closeCB, openWhen, modalRef }) => {
return (
<div>
{openWhen && (
<div ref={modalRef}>
User Search Modal
<button type="button" onClick={closeCB}>Close Modal</button>
</div>
)}
</div>
);
jest.mock('./UserSearchContainer', () => {
return jest.fn(() => <div>UserSearchContainer</div>);
});

const afterCloseMock = jest.fn();
Expand Down Expand Up @@ -86,14 +77,14 @@ describe('PluginFindUser', () => {
renderPluginFindUser(props);
const searchBtn = screen.getByTestId('searchButton');
await userEvent.click(searchBtn);
expect(screen.getByText('User Search Modal')).toBeInTheDocument();
expect(screen.getByText('ui-plugin-find-user.modal.label')).toBeInTheDocument();
});

it('should trigger afterClose() when the modal is closed', async () => {
renderPluginFindUser(props);
const searchBtn = screen.getByTestId('searchButton');
await userEvent.click(searchBtn);
await userEvent.click(screen.getByText('Close Modal'));
expect(afterCloseMock).toHaveBeenCalledTimes(1);
await userEvent.click(screen.getByRole('button', { name : 'stripes-components.dismissModal' }));
await waitFor(() => expect(afterCloseMock).toHaveBeenCalledTimes(1));
});
});
Loading
Loading