UIP-2563 Improve some doc comments #97

aaronlademann-wf · 2017-07-17T19:24:55Z

Misc. documentation improvements I made while doing the memory leak audit.

FYA: @greglittlefield-wf @jacehensley-wf @clairesarsam-wf

aviary-wf · 2017-07-17T19:25:06Z

Raven

Number of Findings: 0

codecov-io · 2017-07-17T19:39:13Z

Codecov Report

Merging #97 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master      #97   +/-   ##
=======================================
  Coverage   94.87%   94.87%           
=======================================
  Files          31       31           
  Lines        1537     1537           
=======================================
  Hits         1458     1458           
  Misses         79       79

jacehensley-wf · 2017-07-17T20:02:48Z

lib/src/util/prop_errors.dart

@@ -14,7 +14,9 @@

 library over_react.prop_errors;

-/// Error thrown dues to a prop being set incorrectly.
+import 'package:over_react/over_react.dart';


Is this only imported for comments?

greglittlefield-wf · 2017-07-18T00:47:07Z

lib/src/component_declaration/component_base.dart

 ///
-/// Extends [react.Component].
+/// __Forwarding when your component renders a composite component:__


s/Forwarding/Prop and CSS classname forwarding

greglittlefield-wf · 2017-07-18T00:48:56Z

lib/src/component_declaration/component_base.dart

 ///
-/// Related: [UiStatefulComponent]
+///       get consumedProps => const [
+///         const $Props(/* Some class containing props you wish to not forward */)


This could be confusing, since the props in the associated props class are automatically forwarded. I think we should only reference this with respect to advanced component examples

greglittlefield-wf · 2017-07-18T00:49:57Z

lib/src/component_declaration/component_base.dart

+///
+///       get consumedProps => const [
+///         const $Props(/* Some class containing props you wish to not forward */)
+///       ];


See above note above about consumedProps

greglittlefield-wf · 2017-07-18T00:53:20Z

lib/src/component_declaration/component_base.dart

@@ -403,28 +474,30 @@ abstract class UiProps extends Object
  @override Map get _map => this.props;
  @override String toString() => '$runtimeType: ${prettyPrintMap(_map)}';

-  /// Adds an arbitrary prop key-value pair if [shouldAdd] is true, otherwise, does nothing.
+  /// Adds an arbitrary prop key-value pair to [props] if [shouldAdd] is true, otherwise, does nothing.


I don't think we should include to [props], since it's not relevant to the readers, and could cause confusion around whether the pair is added to the UiProps instance itself.

I'd say leave it as-is, or change it to to this map or to the backing map.

I think it might be even better to just change the implementation, to reinforce that the current object is a Map.

void addProp(propKey, value, [bool shouldAdd = true]) { if (!shouldAdd) return; this[propKey] = value; }

greglittlefield-wf · 2017-07-18T00:55:04Z

lib/src/component_declaration/component_base.dart

  void addProp(propKey, value, [bool shouldAdd = true]) {
    if (!shouldAdd) return;

    props[propKey] = value;
  }

-  /// Adds a Map of arbitrary props if [shouldAdd] is true and [propMap] is not null.
+  /// Adds a Map of arbitrary props to [props] if [shouldAdd] is true and [propMap] is not null.


See above comment about referencing props.

greglittlefield-wf · 2017-07-18T01:04:50Z

lib/src/util/prop_key_util.dart

@@ -20,7 +20,7 @@ import 'dart:collection';
 ///
 /// Example usage:
 ///
-///     var valuePropKey = getPropKey((props) => props.value, TextInput);
+///     var valuePropKey = getPropKey((props) => props.value, Dom.input);


I think it'd be good to show a custom component here, since that's the main use-case

greglittlefield-wf · 2017-07-18T01:05:21Z

lib/src/util/prop_key_util.dart

@@ -35,7 +35,7 @@ dynamic _getKey(void accessKey(Map keySpy)) {
  return keySpy.key;
 }

-/// Helper class that stores the key accessed while getting a value of a map.
+/// Helper class that stores the key accessed while getting the value of a map.


should be a value within a map

greglittlefield-wf · 2017-07-18T01:06:48Z

lib/src/util/react_wrappers.dart

 /// merged into existing props.
 ///
 /// Handles both Dart and JS React components, returning the appropriate props structure for each type:
 ///
 /// * For Dart components, existing props are read from [InteropProps.internal], which are then merged with
 ///   the new [newProps] and saved in a new [InteropProps] with the expected [ReactDartComponentInternal] structure.
-///   Children are likewise copied/overwritten as expected.
+///   [newChildren] are likewise copied/overwritten as expected.


Should be:

Children are likewise copied and potentially overwritten with [newChildren] as expected.

greglittlefield-wf · 2017-07-18T01:08:05Z

lib/src/util/react_wrappers.dart

@@ -277,7 +274,8 @@ ReactElement cloneElement(ReactElement element, [Map props, Iterable children])
  }
 }

-/// Returns the native Dart component associated with a React JS component instance, or null if the component is not Dart-based.
+/// Returns the native Dart [ReactComponent.dartComponent] associated with a [ReactComponent] instance,


ReactComponent.dartComponent refers to an API that's on a react-dart branch and not yet merged in.

greglittlefield-wf · 2017-07-18T01:08:58Z

lib/src/util/react_wrappers.dart

@@ -277,7 +274,8 @@ ReactElement cloneElement(ReactElement element, [Map props, Iterable children])
  }
 }

-/// Returns the native Dart component associated with a React JS component instance, or null if the component is not Dart-based.
+/// Returns the native Dart [ReactComponent.dartComponent] associated with a [ReactComponent] instance,
+/// or `null` if the component is not Dart-based _(an [Element])_.


s/an [Element]/an [Element] or a JS composite component

aaronlademann-wf · 2017-07-18T13:58:39Z

@greglittlefield-wf feedback addressed.

rmconsole-wf · 2017-07-21T02:06:08Z

@aaronlademann-wf This pull request has merge conflicts, please resolve.

greglittlefield-wf

Just a couple small things, otherwise looks great.

greglittlefield-wf · 2017-07-21T15:37:26Z

lib/src/component_declaration/component_base.dart

@@ -200,7 +245,7 @@ abstract class UiComponent<TProps extends UiProps> extends react.Component imple

  /// A typed props object corresponding to the current untyped props Map ([unwrappedProps]).
  ///
-  /// Created using [typedPropsFactory] and cached for each Map instance.
+  /// Created using [typedPropsFactory] and cached for each Map instance within [_typedPropsCache].


#nit within [_typedPropsCache] is an implementation detail not relevant to consumers of this API (and is also evident by looking at the code).

greglittlefield-wf · 2017-07-21T15:39:51Z

lib/src/component_declaration/component_base.dart

@@ -307,7 +370,7 @@ abstract class UiStatefulComponent<TProps extends UiProps, TState extends UiStat

  /// A typed state object corresponding to the current untyped state Map ([unwrappedState]).
  ///
-  /// Created using [typedStateFactory] and cached for each Map instance.
+  /// Created using [typedStateFactory] and cached for each Map instance within [_typedStateCache].


#nit within [_typedStateCache] is an implementation detail not relevant to consumers of this API (and is also evident by looking at the code).

greglittlefield-wf · 2017-07-21T15:44:43Z

lib/src/util/class_names.dart

  /// populated from the return values of [toClassName] and [toClassNameBlacklist], respectively.
  ///
-  /// This method, along with [addFromProps], is useful for merging sets of className/blacklist props.
+  /// > This method, along with [addFromProps], are useful for merging sets of className/blacklist props.


I believe "is" is correct here, since "along with [addFromProps]" is a separate clause and does not affect the subject of the sentence.

Alternatively, "This method and [addFromProps] are useful" would also be correct.

greglittlefield-wf · 2017-07-21T15:47:22Z

lib/src/util/react_wrappers.dart

@@ -120,7 +117,7 @@ final bool _canUseExpandoOnReactElement = (() {
  return true;
 })();

-/// A cache of props for a given [ReactElement].
+/// A cache of [UiComponent.props] for a given [ReactElement].


This is a little misleading, since not all ReactElement doesn't always have a associated UiComponent (only when it's mounted).

+ That were discovered while doing the memory leak audit

aaronlademann-wf · 2017-07-21T16:00:40Z

@greglittlefield-wf all feedback addressed.

greglittlefield-wf · 2017-07-21T16:09:42Z

+1

clairesarsam-wf · 2017-07-24T16:35:16Z

lib/src/component_declaration/component_type_checking.dart

+  ///     UiFactory<FooProps> Foo;
+  ///
+  ///     @Component(subtypeOf: FooComponent)
+  ///     class BarComponent extends UiComponent<BarProps> {


May be useful to show BarProps extending FooProps here as well

clairesarsam-wf · 2017-07-24T16:39:44Z

+1

jayudey-wf · 2017-07-28T13:24:02Z

QA +1, doc change only

Merging into master

aaronlademann-wf force-pushed the doc_comment_updates branch from 1a0d6bc to 674ea62 Compare July 17, 2017 19:29

jacehensley-wf reviewed Jul 18, 2017

View reviewed changes

greglittlefield-wf reviewed Jul 18, 2017

View reviewed changes

aaronlademann-wf force-pushed the doc_comment_updates branch from 674ea62 to b866700 Compare July 18, 2017 13:57

aaronlademann-wf added the Type: documentation label Jul 18, 2017

rmconsole-wf added the Merge Conflicts label Jul 21, 2017

aaronlademann-wf force-pushed the doc_comment_updates branch from b866700 to 2fa1651 Compare July 21, 2017 14:13

rmconsole-wf removed the Merge Conflicts label Jul 21, 2017

aaronlademann-wf added Merge Conflicts State: needs review and removed Merge Conflicts labels Jul 21, 2017

greglittlefield-wf reviewed Jul 21, 2017

View reviewed changes

Improve some doc comments

2867a22

+ That were discovered while doing the memory leak audit

aaronlademann-wf force-pushed the doc_comment_updates branch from 2fa1651 to 2867a22 Compare July 21, 2017 16:00

rmconsole-wf added the Merge Requirements Met label Jul 21, 2017

clairesarsam-wf reviewed Jul 24, 2017

View reviewed changes

aaronlademann-wf added State: ready for QA and removed State: needs review labels Jul 26, 2017

rmconsole2-wf changed the title ~~Improve some doc comments~~ UIP-2563 Improve some doc comments Jul 28, 2017

jayudey-wf merged commit 4a149e3 into Workiva:master Jul 28, 2017

aaronlademann-wf mentioned this pull request Aug 3, 2017

UIP-2546 Release OverReact 1.15.1 #102

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UIP-2563 Improve some doc comments #97

UIP-2563 Improve some doc comments #97

aaronlademann-wf commented Jul 17, 2017

aviary-wf commented Jul 17, 2017 •

edited by aviary2-wf

Loading

codecov-io commented Jul 17, 2017 •

edited

Loading

jacehensley-wf Jul 17, 2017

aaronlademann-wf Jul 18, 2017

greglittlefield-wf Jul 18, 2017

greglittlefield-wf Jul 18, 2017

greglittlefield-wf Jul 18, 2017

greglittlefield-wf Jul 18, 2017

greglittlefield-wf Jul 18, 2017

greglittlefield-wf Jul 18, 2017

greglittlefield-wf Jul 18, 2017

greglittlefield-wf Jul 18, 2017

greglittlefield-wf Jul 18, 2017

greglittlefield-wf Jul 18, 2017

greglittlefield-wf Jul 18, 2017

aaronlademann-wf commented Jul 18, 2017

rmconsole-wf commented Jul 21, 2017

greglittlefield-wf left a comment

greglittlefield-wf Jul 21, 2017

greglittlefield-wf Jul 21, 2017

greglittlefield-wf Jul 21, 2017

greglittlefield-wf Jul 21, 2017

aaronlademann-wf commented Jul 21, 2017

greglittlefield-wf commented Jul 21, 2017

clairesarsam-wf Jul 24, 2017

clairesarsam-wf commented Jul 24, 2017

jayudey-wf commented Jul 28, 2017

UIP-2563 Improve some doc comments #97

UIP-2563 Improve some doc comments #97

Conversation

aaronlademann-wf commented Jul 17, 2017

aviary-wf commented Jul 17, 2017 • edited by aviary2-wf Loading

Raven

codecov-io commented Jul 17, 2017 • edited Loading

Codecov Report

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

aaronlademann-wf commented Jul 18, 2017

rmconsole-wf commented Jul 21, 2017

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

aaronlademann-wf commented Jul 21, 2017

greglittlefield-wf commented Jul 21, 2017

Choose a reason for hiding this comment

clairesarsam-wf commented Jul 24, 2017

jayudey-wf commented Jul 28, 2017

aviary-wf commented Jul 17, 2017 •

edited by aviary2-wf

Loading

codecov-io commented Jul 17, 2017 •

edited

Loading