UIP-1953 Sync changes from OverReact source #40

clairesarsam-wf · 2017-01-06T23:29:54Z

Ultimate problem:

When OverReact was split out from its original project, updates continued to be made to the original source at the same time updates were being made to the OverReact project.

How it was fixed:

Merge the updates made in the original project with the updates made in the OverReact project.

Testing suggestions:

All tests should pass
Changes made in the original project should now be present in OverReact.

Potential areas of regression:

Changes made to OverReact since it was originally split out should still be present.

Please review @greglittlefield-wf @aaronlademann-wf @jacehensley-wf @joelleibow-wf

aviary-wf · 2017-01-06T23:29:58Z

Raven

Number of Findings: 0

codecov-io · 2017-01-11T23:47:53Z

Current coverage is 97.42% (diff: 97.37%)

Merging #40 into master will decrease coverage by 0.02%

@@             master        #40   diff @@
==========================================
  Files            27         28     +1   
  Lines          1286       1316    +30   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1253       1282    +29   
- Misses           33         34     +1   
  Partials          0          0

Powered by Codecov. Last update 2742bac...030edee

greglittlefield-wf

Some formatting/docs stuff that seems to have regressed, otherwise looks great!

greglittlefield-wf · 2017-01-11T23:47:00Z

lib/src/component_declaration/component_base.dart

-        throw new PropError.required(prop.key, prop.errorMessage);
-      });
+            throw new PropError.required(prop.key, prop.errorMessage);
+          });


#nit looks like this was fixed in over_react after the code diverged

greglittlefield-wf · 2017-01-11T23:47:03Z

lib/src/component_declaration/component_base.dart

@@ -195,11 +203,11 @@ abstract class UiComponent<TProps extends UiProps> extends react.Component {
  // ----------------------------------------------------------------------
 }

-/// The basis for a stateful over_react component.
+/// /// The basis for a stateful over_react component.


#nit looks like this was fixed in over_react after the code diverged

greglittlefield-wf · 2017-01-11T23:47:12Z

lib/src/component_declaration/component_base.dart

 ///
 /// Includes support for strongly-typed props and state and utilities for prop and CSS classname forwarding.
 ///
-/// Extends [react.Component].
+/// Extends [react.Component]


#nit looks like this was fixed in over_react after the code diverged

greglittlefield-wf · 2017-01-11T23:47:15Z

lib/src/component_declaration/component_base.dart

@@ -232,7 +240,6 @@ abstract class UiStatefulComponent<TProps extends UiProps, TState extends UiStat
  set unwrappedState(Map value) => super.state = value;

  /// Returns a typed state object backed by the specified [stateMap].
-  ///


#nit looks like this was fixed in over_react after the code diverged

greglittlefield-wf · 2017-01-11T23:49:00Z

lib/src/component_declaration/flux_component.dart

-  /// Cancellation will be handled automatically by [componentWillUnmount].
+  /// Register a [subscription] that should be canceled when the component
+  /// unmounts. Cancellation will be handled automatically by
+  /// [componentWillUnmount].


Seems like the version of this file was more up to date in over_react (i.e., these changes should be discarded)

greglittlefield-wf · 2017-01-11T23:50:35Z

lib/src/util/document_event_helper_util.dart

@@ -0,0 +1,16 @@
+library document_event_helper;


Not sure this should be added

+1 - this remains in ui_core, so there's no reason to have it here.

There are things that were part of ui_core that we didn't bring over, and this is one of them.

It's part of the public API maintain, so we shouldn't add it unless there IS a reason to have it here, IMO.

greglittlefield-wf · 2017-01-11T23:50:48Z

lib/src/util/js_util.dart

@@ -0,0 +1,14 @@
+library over_react.js_util;


Not sure this should be added

greglittlefield-wf · 2017-01-11T23:51:13Z

lib/src/util/rem_util.dart

@@ -21,7 +21,6 @@ import 'dart:html';
 import 'package:over_react/over_react.dart';
 import 'package:over_react/src/util/css_value_util.dart';
 import 'package:react/react_dom.dart' as react_dom;
-


#Nit there should be a space here

greglittlefield-wf · 2017-01-11T23:51:24Z

lib/src/util/test_mode.dart

@@ -13,7 +13,6 @@
 // limitations under the License.

 library over_react.test_mode;
-


#nit there should be a space here

greglittlefield-wf · 2017-01-11T23:52:10Z

...act/component_declaration/transformer_integration_tests/accessor_mixin_integration_test.dart

@@ -50,7 +50,7 @@ main() {
        });
      });

-      group('generates prop getters/setters, when there is a custom key namespace, with', () {
+          group('generates prop getters/setters, when there is a custom key namespace, with', () {


#nit Is this indentation right?

jacehensley-wf

Done with my first pass. Just noticed a few things.

jacehensley-wf · 2017-01-11T23:50:39Z

lib/src/component_declaration/component_base.dart

+  ///
+  /// Overridden for strong typing.
+  @override
+  _RefTypedef get ref => super.ref;


I thought we removed these redeclaration of refs when we added the typing to react-dart.

jacehensley-wf · 2017-01-11T23:52:25Z

lib/src/util/react_wrappers.dart

+///
+/// Deprecated: Simply call `isMounted` on the [ReactComponent] instead.
+@Deprecated('2.0.0')
+bool isMounted(ReactComponent instance) => instance.isMounted();


I don't think this needs to be added

jacehensley-wf · 2017-01-11T23:53:12Z

test/over_react/component_declaration/flux_component_test/handler_precedence.dart

@@ -20,7 +20,7 @@ class TestHandlerPrecedenceComponent extends FluxUiComponent<TestHandlerPreceden
  @override
  getStoreHandlers() => {props.store.store1: increment};

-  increment(Store store) {


I think this had to be typed for strong mode.

aaronlademann-wf · 2017-01-12T18:10:24Z

test/over_react/component_declaration/flux_component_test/store_handlers.dart

@@ -16,7 +16,7 @@ class TestStoreHandlersComponent extends FluxUiComponent<TestStoreHandlersProps>
  @override
  getStoreHandlers() => {props.store: increment};

-  increment(Store store) {
+  increment(_) {


This should not have been changed

aaronlademann-wf · 2017-01-12T18:10:47Z

test/over_react/component_declaration/flux_component_test/store_handlers.dart

@@ -7,7 +7,7 @@ UiFactory<TestStoreHandlersProps> TestStoreHandlers;
 class TestStoreHandlersProps extends FluxUiProps<TestActions, TestStore> {}

 @Component()
-class TestStoreHandlersComponent extends FluxUiComponent<TestStoreHandlersProps> {
+class TestStoreHandlersComponent extends FluxUiComponent {


This should not have been changed (You should see errors in your analysis server view @clairesarsam-wf - do you not?)

aaronlademann-wf · 2017-01-12T18:10:56Z

test/over_react/component_declaration/flux_component_test/handler_precedence.dart

@@ -7,7 +7,7 @@ UiFactory<TestHandlerPrecedenceProps> TestHandlerPrecedence;
 class TestHandlerPrecedenceProps extends FluxUiProps<TestActions, TestStores> {}

 @Component()
-class TestHandlerPrecedenceComponent extends FluxUiComponent<TestHandlerPrecedenceProps> {
+class TestHandlerPrecedenceComponent extends FluxUiComponent {


This should not have been changed

aaronlademann-wf · 2017-01-12T18:11:13Z

...act/component_declaration/transformer_integration_tests/accessor_mixin_integration_test.dart

@@ -81,7 +81,7 @@ main() {
      });
    });

-    group('@StateMixin()', () {
+      group('@StateMixin()', () {


This should not have been changed.

aaronlademann-wf · 2017-01-12T18:11:19Z

...act/component_declaration/transformer_integration_tests/accessor_mixin_integration_test.dart

@@ -112,7 +112,7 @@ main() {
        });
      });

-      group('generates state getters/setters, when there is a custom key namespace, with', () {
+          group('generates state getters/setters, when there is a custom key namespace, with', () {


This should not have been changed

aaronlademann-wf · 2017-01-12T18:14:36Z

test/over_react/util/rem_util_test.dart

-          );
-        });
-
-        test('unless `passThroughUnsupportedUnits` is true', () {


I believe this parameter is intentional - shouldn't this test remain?

aaronlademann-wf · 2017-01-12T18:14:45Z

test/over_react/util/rem_util_test.dart

-          );
-        });
-
-        test('unless `passThroughUnsupportedUnits` is true', () {


I believe this parameter is intentional - shouldn't this test remain?

Revert formatting/content changes that shouldn't have been merged in

clairesarsam-wf · 2017-01-13T17:40:27Z

@jacehensley-wf @aaronlademann-wf @greglittlefield-wf Feedback has been addressed - all warnings/infos are gone on dart 1.21.0.

jacehensley-wf

Just one small thing, otherwise looks great.

jacehensley-wf · 2017-01-13T18:18:55Z

test/over_react/component_declaration/component_base_test.dart

@@ -92,7 +92,7 @@ main() {
        test('a single child is passed in', () {
          var child = 'Only child';
          var renderedNode = renderAndGetDom(Dom.div()(child));
-          List<Text> children = renderedNode.childNodes.where((node) => node.nodeType != Node.COMMENT_NODE).toList();
+           List<Text> children = renderedNode.childNodes.where((node) => node.nodeType != Node.COMMENT_NODE).toList();


Indentation is off here.

clairesarsam-wf · 2017-01-17T16:24:20Z

@jacehensley-wf Feedback addressed

greglittlefield-wf · 2017-01-17T22:51:33Z

+1

jacehensley-wf · 2017-01-17T22:57:12Z

+1

aaronlademann-wf · 2017-01-23T14:39:57Z

+1

aaronlademann-wf · 2017-01-23T14:44:14Z

@rmconsole-wf go

leviwith-wf · 2017-01-23T17:46:32Z

QA Resource Approval: +10

Merging

clairesarsam-wf added the Hold Hold merges label Jan 6, 2017

clairesarsam-wf force-pushed the sync_changes_2 branch 5 times, most recently from 05ce9de to 341e680 Compare January 10, 2017 18:50

clairesarsam-wf mentioned this pull request Jan 11, 2017

CP-2912 Release platform_detect 1.1.1 Workiva/platform_detect#12

Merged

clairesarsam-wf force-pushed the sync_changes_2 branch 2 times, most recently from 140912a to 08ce78d Compare January 11, 2017 21:49

Sync changes from source repository onto over_react

08ce78d

clairesarsam-wf removed the Hold Hold merges label Jan 11, 2017

greglittlefield-wf reviewed Jan 11, 2017

View reviewed changes

jacehensley-wf suggested changes Jan 11, 2017

View reviewed changes

aaronlademann-wf added the Type: task label Jan 12, 2017

aaronlademann-wf reviewed Jan 12, 2017

View reviewed changes

clairesarsam-wf force-pushed the sync_changes_2 branch 4 times, most recently from 69f67dd to 8fb0ba0 Compare January 13, 2017 17:35

Address CR feedback

8fb0ba0

Revert formatting/content changes that shouldn't have been merged in

jacehensley-wf reviewed Jan 13, 2017

View reviewed changes

Correct indentation

030edee

rmconsole-wf added the Merge Requirements Met label Jan 17, 2017

jacehensley-wf approved these changes Jan 17, 2017

View reviewed changes

jacehensley-wf added the Hold Hold merges label Jan 17, 2017

aaronlademann-wf added State: ready for QA and removed Hold Hold merges labels Jan 23, 2017

aaronlademann-wf changed the title ~~Sync changes from OverReact source~~ UIP-1953 Sync changes from OverReact source Jan 23, 2017

leviwith-wf merged commit 94baf47 into Workiva:master Jan 23, 2017

aaronlademann-wf mentioned this pull request Jun 29, 2020

Add a "Forward unconsumed props" assist to analyzer plugin #590

Open

		@@ -13,7 +13,6 @@
		// limitations under the License.

		library over_react.test_mode;

UIP-1953 Sync changes from OverReact source #40

UIP-1953 Sync changes from OverReact source #40

Conversation

clairesarsam-wf commented Jan 6, 2017 • edited Loading

Ultimate problem:

How it was fixed:

Testing suggestions:

Potential areas of regression:

aviary-wf commented Jan 6, 2017

Raven

codecov-io commented Jan 11, 2017 • edited Loading

Current coverage is 97.42% (diff: 97.37%)

greglittlefield-wf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacehensley-wf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clairesarsam-wf commented Jan 13, 2017

jacehensley-wf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clairesarsam-wf commented Jan 17, 2017

greglittlefield-wf commented Jan 17, 2017

jacehensley-wf commented Jan 17, 2017

aaronlademann-wf commented Jan 23, 2017

aaronlademann-wf commented Jan 23, 2017

leviwith-wf commented Jan 23, 2017

clairesarsam-wf commented Jan 6, 2017 •

edited

Loading

codecov-io commented Jan 11, 2017 •

edited

Loading