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.

Sign up for GitHub

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

Jump to bottom

Fixed correct original props callback on cloneElement #15990

Merged
merged 5 commits into from
Mar 27, 2023
Merged

Fixed correct original props callback on cloneElement #15990

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4f81115
Fixed correct original props callback on cloneElement
s77rt Mar 15, 2023
c4d8509
Fixed cloneElement bug on Form.js
s77rt Mar 17, 2023
8b2cc78
Update src/components/Form.js
s77rt Mar 22, 2023
63bde06
Merge branch 'main' into cloneElement-call-original-props
s77rt Mar 22, 2023
17db507
Merge branch 'main' into cloneElement-call-original-props
s77rt Mar 26, 2023
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
7 changes: 5 additions & 2 deletions src/components/Form.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,22 +272,25 @@ class Form extends React.Component {
ref: (node) => {
this.inputRefs[inputID] = node;

// Call the original ref, if any
const {ref} = child;
if (_.isFunction(ref)) {
Copy link
Contributor

@cead22 cead22 Mar 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)

We do this check in many places, where we check if something is a function and if it is we invoke it. Should we replace these instances with Func.invoke like Func.invoke(ref)?
https://github.com/Expensify/expensify-common/blob/36d6f23/lib/Func.jsx#L13

Copy link
Contributor Author

@s77rt s77rt Mar 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is not necessary a repeated logic. Although Func.invoke makes the block of code in one line, it may be confusing; We will have to call Func.invoke(undefined, [ref]). Notice that the arguments must be in an array which is an easy thing to miss.

Copy link
Contributor

Choose a reason for hiding this comment

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

I won't block on this, since this duplication is already here, but here's why I don't think I agree, just for context

  • Just in this PR, we could replace 3 lines of code with 1, in 5 places, so I don't see why this wouldn't be considered repeated logic
  • We'll have to call Func.invoke(red, [node]) in this case, the params being in an array is as easy to miss as any other piece of code, like passing node to ref(node). Passing arguments to a function in an array is pretty standard in JS when using Function.prototype.apply

ref(node);
}
},
value: this.state.inputValues[inputID],
errorText: this.state.errors[inputID] || fieldErrorMessage,
onBlur: () => {
onBlur: (event) => {
// We delay the validation in order to prevent Checkbox loss of focus when
// the user are focusing a TextInput and proceeds to toggle a CheckBox in
// web and mobile web platforms.
setTimeout(() => {
this.setTouchedInput(inputID);
this.validate(this.state.inputValues);
}, 200);

if (_.isFunction(child.props.onBlur)) {
child.props.onBlur(event);
}
},
onInputChange: (value, key) => {
const inputKey = key || inputID;
Expand Down
18 changes: 6 additions & 12 deletions src/components/Hoverable/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,30 +69,24 @@ class Hoverable extends Component {
onMouseEnter: (el) => {
this.setIsHovered(true);

// Call the original onMouseEnter, if any
const {onMouseEnter} = this.props.children;
if (_.isFunction(onMouseEnter)) {
onMouseEnter(el);
if (_.isFunction(this.props.children.props.onMouseEnter)) {
this.props.children.props.onMouseEnter(el);
}
},
onMouseLeave: (el) => {
this.setIsHovered(false);

// Call the original onMouseLeave, if any
const {onMouseLeave} = this.props.children;
if (_.isFunction(onMouseLeave)) {
onMouseLeave(el);
if (_.isFunction(this.props.children.props.onMouseLeave)) {
this.props.children.props.onMouseLeave(el);
}
},
onBlur: (el) => {
if (!this.wrapperView.contains(el.relatedTarget)) {
this.setIsHovered(false);
}

// Call the original onBlur, if any
const {onBlur} = this.props.children;
if (_.isFunction(onBlur)) {
onBlur(el);
if (_.isFunction(this.props.children.props.onBlur)) {
this.props.children.props.onBlur(el);
}
},
});
Expand Down
6 changes: 2 additions & 4 deletions src/components/Tooltip/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,10 +172,8 @@ class Tooltip extends PureComponent {
onBlur: (el) => {
this.hideTooltip();

// Call the original onBlur, if any
const {onBlur} = this.props.children;
if (_.isFunction(onBlur)) {
onBlur(el);
if (_.isFunction(this.props.children.props.onBlur)) {
this.props.children.props.onBlur(el);
}
},
focusable: true,
Expand Down