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

React LabelGroup linking fix #271

Merged
merged 5 commits into from
Jan 13, 2023
Merged

React LabelGroup linking fix #271

merged 5 commits into from
Jan 13, 2023

Conversation

ellthompson
Copy link
Contributor

React LabelGroup fields were not being linked to an observer when a link property was passed in. Updates to PCUI components wrapped in a LabelGroup should now correctly update their bound observer.

@ellthompson ellthompson added the bug Something isn't working label Jan 12, 2023
@ellthompson ellthompson requested a review from a team January 12, 2023 16:10
@ellthompson ellthompson self-assigned this Jan 12, 2023
@@ -16,13 +16,17 @@ class LabelGroup extends BaseComponent <LabelGroupArgs, any> {
if (Array.isArray(this.props.children) || !this.props.children) {
throw new Error('A LabelGroup must contain a single child react component');
}
const labelFieldChild = (this.props.children as { type: any, props: any });
Copy link
Contributor

@willeastcott willeastcott Jan 12, 2023

Choose a reason for hiding this comment

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

Not straightforward to provide types here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the types and included an explanation.

@ellthompson
Copy link
Contributor Author

I've updated this PR to include fixes for #270. Re-requesting a review.

throw new Error(childrenErrorMessage);
}
// casting child as a single ReactElement as we have confirmed it is above
const child = this.props.children as ReactElement;
Copy link
Contributor

@willeastcott willeastcott Jan 12, 2023

Choose a reason for hiding this comment

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

Why is the name of the property children if it's a single child? Looks a bit odd.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This children prop is provided by react and includes any child react elements a user has added to their LabelGroup. It's then been limited to a single react element above this line.

@ellthompson ellthompson merged commit 425684e into main Jan 13, 2023
@ellthompson ellthompson deleted the label-group-react-fix branch January 13, 2023 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants