Skip to content

CPLAT-3968 CPLAT-3969 Add unconvertJsProps() util to react_client.dart#153

Merged
greglittlefield-wf merged 6 commits intoWorkiva:masterfrom
evanweible-wf:unconvertJsProps
Feb 8, 2019
Merged

CPLAT-3968 CPLAT-3969 Add unconvertJsProps() util to react_client.dart#153
greglittlefield-wf merged 6 commits intoWorkiva:masterfrom
evanweible-wf:unconvertJsProps

Conversation

@evanweible-wf
Copy link
Copy Markdown
Collaborator

@evanweible-wf evanweible-wf commented Feb 6, 2019

Description

The getJsProps() method in over_react makes more sense in this library, as it is intended to be used only with JS components and not Dart components. over_react has its own getProps() that should be sufficient (and uses getJsProps() internally).

To avoid naming conflicts and ambiguous imports in over_react, I've added the same functionality under a different name: unconvertJsProps(). This should also help clarify its intended usage in the future when some additional JS map work is taken on.

Additionally, logic was added to handle the unconverting of the JS event handlers (they are converted to their synthetic counterparts when added), which is necessary for Dart 2 compatibility.

Testing

  • CI passes
  • Verify this branch with over_react (on dart 1 and dart 2) as well as any other internal consumer large enough to instill adequate confidence

Code Review

@greglittlefield-wf @aaronlademann-wf @kealjones-wk @corwinsheahan-wf

@greglittlefield-wf
Copy link
Copy Markdown
Collaborator

+1

@evanweible-wf
Copy link
Copy Markdown
Collaborator Author

@greglittlefield-wf I went ahead and added a dartfmt check to CI since we recently formatted this repo

@greglittlefield-wf
Copy link
Copy Markdown
Collaborator

+1

for (final key in eventPropKeyToEventFactory.keys) {
expect(jsProps[key], isNotNull,
reason: 'JS event handler prop should not be null');
expect(identical(jsProps[key], originalHandlers[key]), isTrue,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

FYI there's a same matcher you can use

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Can't believe I didn't know that

@greglittlefield-wf
Copy link
Copy Markdown
Collaborator

+1

@evanweible-wf
Copy link
Copy Markdown
Collaborator Author

@greglittlefield-wf
Copy link
Copy Markdown
Collaborator

+1, merging

@greglittlefield-wf greglittlefield-wf merged commit 25ea39e into Workiva:master Feb 8, 2019
@evanweible-wf evanweible-wf deleted the unconvertJsProps branch February 8, 2019 16:18
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.

3 participants