Skip to content

CPLAT-3874: Function Components #221

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

Merged
merged 55 commits into from
Nov 13, 2019
Merged

Conversation

kealjones-wk
Copy link
Collaborator

@kealjones-wk kealjones-wk commented Oct 24, 2019

Motivation

Support Function Components in react-dart

Changes

  • Added ReactDartFunctionComponentFactoryProxy class
  • Added registerFunctionComponent method
  • Abstracted ReactDartComponentFactoryProxy2's build and generateExtendedJsProps logic into mixin for re-use with ReactDartFunctionComponentFactoryProxy
  • Updated isDartComponent logic to support function components
  • Added tests

- used to force null returns to be javascript null, dart2js sometimes converts null to undefined and react doesn't like that, not one tiny bit.
@kealjones-wk kealjones-wk marked this pull request as ready for review October 28, 2019 20:59
@kealjones-wk kealjones-wk changed the title WIP: CPLAT-3874: Function Components CPLAT-3874: Function Components Oct 28, 2019
Copy link
Collaborator

@joebingham-wk joebingham-wk left a comment

Choose a reason for hiding this comment

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

This looks really solid. The actual implementation is really simple and elegant. I just found a few little things, mostly nits!

@aaronlademann-wf aaronlademann-wf modified the milestones: 5.1.0, 5.2.0 Nov 5, 2019
@aaronlademann-wf aaronlademann-wf changed the base branch from 5.1.0-wip to 5.2.0-wip November 5, 2019 21:46
@kealjones-wk kealjones-wk force-pushed the CPLAT-3874-function-components branch from 7b4e5b8 to 40b9ef2 Compare November 5, 2019 22:24
Copy link
Collaborator

@aaronlademann-wf aaronlademann-wf left a comment

Choose a reason for hiding this comment

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

Just one nit on a confusing doc comment... but I'm +1 on this.

Copy link
Collaborator

@joebingham-wk joebingham-wk left a comment

Choose a reason for hiding this comment

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

This looks awesome! I found a couple import sections that could be alphabetical, but those are super nitty

Copy link
Collaborator

@joebingham-wk joebingham-wk left a comment

Choose a reason for hiding this comment

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

+1

@aaronlademann-wf
Copy link
Collaborator

+1 refresh

Copy link
Collaborator

@joebingham-wk joebingham-wk left a comment

Choose a reason for hiding this comment

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

  • Tested function components in the example

+10

Copy link
Collaborator

@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.

Two small comments plus some merge-conflicts and a related comment 😢

Copy link
Collaborator

@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

Copy link
Collaborator

@joebingham-wk joebingham-wk left a comment

Choose a reason for hiding this comment

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

+10

@kealjones-wk kealjones-wk merged commit f808ac8 into 5.2.0-wip Nov 13, 2019
@greglittlefield-wf greglittlefield-wf deleted the CPLAT-3874-function-components branch November 21, 2019 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants