-
Notifications
You must be signed in to change notification settings - Fork 3
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
Changes from all commits
0e79040
e897582
067f3c3
f69ba71
dd9e990
3004712
0ccbd43
c6abc99
cd00fed
1593cd4
6a719e0
6c69d33
66cc3ab
4aee2af
eca572e
74310fe
16ffb28
99a59f3
5ce77ef
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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'; | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are unused variables specified? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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]); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think adding There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? |
||
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 = { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving class components to functional ones seems redundant doc.
Is there any reason to touch class components?
There was a problem hiding this comment.
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.