#72 Now is possibile to pass IRenderFunction to component Render* #83

MarcOne68 · 2019-01-27T09:35:07Z

I defined the class RenderOverride.
You can create an instance of this class passing a IRenderFunction<...> and set the Render* property of a component with that instance.

msftclas · 2019-01-27T09:35:18Z

All CLA requirements met.

bengry

@MarcOne68 Can you give an example use-case for this change + the code in the app that would use it? Why would you prefer to write your own function rather than return one of the already-available options (TemplateRef/ComponentRef/function returning an HTML element).

bengry · 2019-01-27T12:10:57Z

libs/core/src/lib/components/wrapper-component.ts

    }

+    if (input instanceof RenderOverride) {
+      return undefined;


why not return input.renderer?

How I wrote in Issue #72, these changes help me to set the styles
This is the code example:

private overrideRenderRow(props: IDetailsRowProps, defaultRender: (props: IDetailsRowProps) => JSX.Element): JSX.Element { props.styles = MY_STYLES; return defaultRender(props); }

Sounds reasonable - you want to expose the native JSX to Angular-land then. This should be fine for simple cases, since JSX in Angular is something I’d like to avoid (although possible of course).

However, I suggest we change the API, to avoid the boilerplate of creating a class (which would really only have one instance), if possible.
I want to think about an API further, and see if we can somehow avoid exposing JSX to the final user (i.e. the developer using @angular-react/fabric) - perhaps allowing to pass a function that accepts TProps and returns TProps, which will then be fed to the defaultRenderer that office-ui-fabric-react exposes. If you have an idea for an API - I’m open for suggestions, and how it would work.

clicked approve by mistake

bengry · 2019-01-27T22:00:30Z

libs/core/src/lib/components/wrapper-component.ts

 import { toObject } from '../utils/object/to-object';
 import { afterRenderFinished } from '../utils/render/render-delay';
 import { unreachable } from '../utils/types/unreachable';
+import { IRenderFunction } from 'office-ui-fabric-react';


Please don’t import stuff from office-ui-fabric-react inside the @angular-react/core package - we want to keep it library-agnostic.

A more generic type is needed here instead therefore.

bengry

Please see my comments

bengry · 2019-01-28T13:59:10Z

libs/core/src/lib/components/wrapper-component.ts

-export class RenderOverride<TContext extends object> {
-  constructor(public renderer: IRenderFunction<TContext>) { }
+export interface DefaultRenderOptions<TContext extends object> {
+  readonly getProps: (props?: TContext) => TContext;


maybe rename to defaultProps, to make it clearer.

bengry · 2019-01-28T14:02:21Z

libs/core/src/lib/components/wrapper-component.ts

-
-      // Call the function again with the created ComponentRef<TContext>
-      return this.createInputJsxRenderer(componentRef, additionalProps);
+      if ('componentType' in input) {


please replace this with a user defined type guard, this function does too much at the moment (it was bearable so far since it only had to deal with one object, but in hindsight it should've extracted the "is this a RenderComponentOptions" to a user defined type guard.

Also, you want to check with hasOwnProperty, since it doesn't look up the prototype chain.

bengry · 2019-01-28T14:02:44Z

libs/core/src/lib/components/wrapper-component.ts

+        // Call the function again with the created ComponentRef<TContext>
+        return this.createInputJsxRenderer(componentRef, additionalProps);
+      } else {
+        return undefined;


why is this undefined in the DefaultRenderOptions case?

I understand the technical reason of why needing to handle this case is here - but why is returning undefined specifically? Shouldn't there be a render function call here, or rather, a function that creates a render function.

Only because unreachable(input) throw an error during the compilation.
The execution never reach that line of code because the method createRenderPropHandler executes the cases RenderPropOptions and RenderPropContext.

bengry · 2019-01-28T14:04:44Z

libs/core/src/lib/components/wrapper-component.ts

  ): (props?: TProps, defaultRender?: JsxRenderFunc<TProps>) => JSX.Element | null {
-    if (renderInputValue instanceof RenderOverride) {
-      return renderInputValue.renderer;
+    if ((typeof renderInputValue === 'object') && ('getProps' in renderInputValue)) {


Please use a user-defined type guard here, as mentioned in the other comment. Making one will also make sure you can re-use the logic of it in multiple places.

bengry · 2019-01-28T14:09:03Z

libs/core/src/lib/components/wrapper-component.ts


-export class RenderOverride<TContext extends object> {
-  constructor(public renderer: IRenderFunction<TContext>) { }
+export interface DefaultRenderOptions<TContext extends object> {


Thinking out load here - the DefaultRenderOptions doesn't convey the message that it allows mutating the props. If you have suggestions for a better/other name - I'm open for suggestions, but if not - please add a JDoc comment explaining what the purpose of this object is.

bengry · 2019-01-29T07:35:10Z

libs/fabric/src/lib/components/text-field/base-text-field.component.ts

  @Output() readonly onChange = new EventEmitter<{ event: Event; newValue?: string }>();
  @Output() readonly onBeforeChange = new EventEmitter<{ newValue: any }>();
  @Output() readonly onNotifyValidationResult = new EventEmitter<{ errorMessage: string; value: string | undefined }>();
+  @Output() readonly onBlur = new EventEmitter<{ event: Event }>();


Please move these changes to a separate PR. It makes it easier to maintain and create the changelog (this part of the PR looks fine as it is, so if you move it it can get it as-is)

OK, I'll try to do it but.... I don't know so well GIT :)

Basically create a new branch from master and make the same changes there (or you can cherry-pick the commit using git cherry-pick 95e1991 when you're on the new branch.
Then create a new PR from it.

bengry · 2019-03-02T10:28:41Z

@MarcOne68 This is a good PR (or two, as I mentioned in another comment).
Is this something you're still working on and would like to merge? It's been over a month since the last change here.

@MarcOne68

Continuing work done in #83 by @MarcOne68

bengry · 2019-03-25T15:43:52Z

Implemented as part of #92 and #99.

#72 Use defaultRender

4528b31

bengry previously approved these changes Jan 27, 2019

View reviewed changes

bengry reviewed Jan 27, 2019

View reviewed changes

Merge branch 'master' into master

9025ea3

bengry self-assigned this Jan 28, 2019

bengry suggested changes Jan 28, 2019

View reviewed changes

MarcOne68 added 2 commits January 28, 2019 14:04

#72 Request Change

f0d1b3f

Merge branch 'master' of https://github.com/MarcOne68/angular-react

9bc39eb

bengry suggested changes Jan 28, 2019

View reviewed changes

MarcOne68 added 2 commits January 28, 2019 17:42

#72

4e5739d

Added onBlur to TextField (future ControlValueAccessor implementation)

95e1991

bengry suggested changes Jan 29, 2019

View reviewed changes

bengry mentioned this pull request Mar 14, 2019

add blur events to fabric text field components #98

Merged

bengry added a commit that referenced this pull request Mar 14, 2019

add blur events to fabric text field components (#98)

8e997c9

Continuing work done in #83 by @MarcOne68

bengry mentioned this pull request Mar 14, 2019

Allow modifying props in custom render function #99

Merged

bengry closed this Mar 25, 2019

#72 Now is possibile to pass IRenderFunction to component Render* #83

#72 Now is possibile to pass IRenderFunction to component Render* #83

Uh oh!

Conversation

MarcOne68 commented Jan 27, 2019

Uh oh!

msftclas commented Jan 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bengry left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bengry left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bengry Jan 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bengry commented Mar 2, 2019

Uh oh!

bengry commented Mar 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

msftclas commented Jan 27, 2019 •

edited

Loading

bengry Jan 28, 2019 •

edited

Loading