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

CPLAT-9327 Improve forwardRef documentation, add displayName argument #454

Merged

Conversation

aaronlademann-wf
Copy link
Contributor

DEPENDS ON UNRELEASED CHANGES IN REACT-DART

Motivation

While grokking our forwardRef API, I found myself wanting some implementation examples which could not be found in the documentation comment.

I also noticed that consumers had no way to define a displayName for the wrapper HOC created by forwardRef.

Changes

These changes address both issues by adding extensive examples in the documentation comment for forwardRef that mirror the analogous ReactJS forwardRef documentation, and by adding a displayName argument to the function which then gets set on the JS side via a call to defineProperty.

Release Notes

Allow custom displayName values for forwardRef wrapper components.

Review

See CONTRIBUTING.md for more details on review types (+1 / QA +1 / +10) and code review process.

Please review: @greglittlefield-wf @joebingham-wk @sydneyjodon-wk

QA Checklist

  • Tests were updated and provide good coverage of the changeset and other affected code
  • Manual testing was performed if needed

Merge Checklist

While we perform many automated checks before auto-merging, some manual checks are needed:

  • A Client Platform member has reviewed these changes
  • There are no unaddressed comments - this check can be automated if reviewers use the "Request Changes" feature
  • For release PRs - Version metadata in Rosie comment is correct

@aviary2-wf
Copy link

Security Insights

No security relevant content was detected by automated scans.

Action Items

  • Review PR for security impact; comment "security review required" if needed or unsure
  • Verify aviary.yaml coverage of security relevant code

Questions or Comments? Reach out on Slack: #support-infosec.

@aaronlademann-wf aaronlademann-wf changed the title Improve forwardRef documentation, add displayName argument CPLAT-9327 Improve forwardRef documentation, add displayName argument Jan 23, 2020
@aaronlademann-wf aaronlademann-wf force-pushed the improve-forwardref-documentation branch from 05a0662 to 9c8825e Compare January 23, 2020 20:19
Copy link
Contributor

@greglittlefield-wf greglittlefield-wf left a comment

Choose a reason for hiding this comment

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

+1

@greglittlefield-wf
Copy link
Contributor

Also, should this go into a function components WIP branch if it's dependent on changes in the react-dart 5.4.0-wip?

@smaifullerton-wk smaifullerton-wk marked this pull request as ready for review July 22, 2020 16:54
///
/// UiFactory<LogPropsProps> _LogProps = _$_LogProps;
///
/// mixin LogPropsProps on UiProps {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please double check the boilerplate here - I updated this example from what it was previously.

@aaronlademann-wf
Copy link
Contributor Author

@smaifullerton-wk I don't think the stuff you need from the react package is in the 5.4.0 release. You'll still need to have that 5.4.0/improve-forwardref-examples-and-typing dep override until its released.

@aaronlademann-wf
Copy link
Contributor Author

Also, should this go into a function components WIP branch if it's dependent on changes in the react-dart 5.4.0-wip?

@greglittlefield-wf is there a function components wip branch here? I see both this - and Sydney's function component PRs with a base branch of master...

@greglittlefield-wf
Copy link
Contributor

@aaronlademann-wf I don't believe there is one, no, but we could make one!

@aaronlademann-wf aaronlademann-wf changed the base branch from master to function-components-wip July 23, 2020 15:13
@aaronlademann-wf
Copy link
Contributor Author

@greglittlefield-wf created the branch / updated the base branch of this PR. (I'll update Sydney's as well)

Copy link
Contributor

@greglittlefield-wf greglittlefield-wf left a comment

Choose a reason for hiding this comment

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

+10 except for CI failing; looks like generated files just need to be updated (pbr build + commit)

Copy link
Contributor

@greglittlefield-wf greglittlefield-wf left a comment

Choose a reason for hiding this comment

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

+10

@Workiva/release-management-p

@smaifullerton-wk smaifullerton-wk merged commit a35d59f into function-components-wip Jul 29, 2020
@smaifullerton-wk smaifullerton-wk deleted the improve-forwardref-documentation branch July 29, 2020 23:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants