Skip to content
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

How to correctly implement custom components (col and row at custom components within a GridLayout) #1422

Open
yuri-becker opened this issue Jul 10, 2018 · 35 comments

Comments

@yuri-becker
Copy link

Hey everyone,

when working with own components in a GridLayout I noticed that properties like row, col and colSpan are not working on the custom component (aswell as class, flexGrow, alignSelf, etc.). I mean, it makes sense since those properties are not defined on my custom component.

For instance, simple stuff like this would render both the Label and my-cool-component in the first row of the GridLayout, since the layout defaults row and column back to 0.

import { Component, Input } from "@angular/core";

@Component({
    selector: 'my-cool-component',
    template: `<Label text="I am in the second row."></Label>`
})
export class MyCoolComponent {
}

@Component({
    selector: "ns-app",
    template: `<GridLayout rows="100, 100, 100", columns="*">
        <Label text="I am in the first row" row="0"></Label>
        <my-cool-component row="1"></my-cool-component>
        <Button text="I am in the third row" row="2"></Button>
    </GridLayout>
    `
})
export class AppComponent { 

}

How can I make these properties work with own components though?

  • Looking at the ts-declaration files of NativeScript's components (for instance tns-core-modules/ui/text-view/text-view.d.ts) reveals that all of them inhererit from ViewBase. In ViewBase properties like row and col and defined. Simply inherting my component from ViewBase (export class MyCoolComponent extends ViewBase) does not make these properties work for the GridLayout.
  • Defining the properties via @input does not work either (export class MyCoolComponent { @Input() col: number; @Input() row: number; }) for the GridLayout.
  • The ugly solution would be to wrap my-cool-component in a, for instance, StackLayout and set row and col on the StackLayout.

Is there any way I can make my component defining row and col with the outer GridLayout using these properties (same goes for all the other propreties the components included in NativeScript have)? Did I somehow misunderstand what components are there for? Is there a piece of documentation which i just didnt see and points to a solution of my problem?

@farfromrefug
Copy link

I am actually facing the same issue. I tried to make my component extends ui/layouts/stack-layout. The issue is col or row are not input from there.

That s a really important feature

@breningham
Copy link

If you implement the @input rules for your custom component, and then add [row]="row" [col]="col" [colSpan]="colSpan" [rowSpan]="rowSpan" to the base element/view of your component

@farfromrefug
Copy link

@breningham You mean doing @Input() colSpan ? Cause the point of that issue is just not to have to do that as it handled by the parent class.

@yuri-becker
Copy link
Author

@breningham
Do you mean like this?

@Component({
    selector: 'my-cool-component',
    template: `<Label [row]="row" text="I am in the second row."></Label>`
})
export class MyCoolComponent {
    @Input() row: number;
}

@Component({
    selector: "ns-app",
    template: `<GridLayout rows="100, 100, 100", columns="*">
        <Label text="I am in the first row" row="0"></Label>
        <my-cool-component row="1"></my-cool-component>
        <Button text="I am in the third row" row="2"></Button>
    </GridLayout>
    `
})
export class AppComponent {

}

Sadly, this doesnt work either. If it would work though, it would kinda result in a lot of boilerplate.

@breningham
Copy link

breningham commented Jul 12, 2018 via email

@farfromrefug
Copy link

Also it does not work and you should not need to do [row]="row" otherwise you would have to do it for all properties....

@darkyelox
Copy link

We need some explanation for using this in Angular because the Big deal of Angular is the reusable Components and NativeScript don't have any docs about it, that is sad for this great framework, we need this feature ASAP.

@NickIliev
Copy link

@mailiam
Copy link

mailiam commented Nov 28, 2018

Another possible workaround is to registerElement with appropriate tns-core-module and use ng-container, HostBinding

for example @maik-becker component could work with:

// register component host with NativeScript Label
registerElement('my-cool-component', () => require('tns-core-modules/ui/label').Label)

@Component({
    selector: 'my-cool-component',
    template: `<ng-container></ng-container>`
})
export class MyCoolComponent {
    // access Label[text] host property
    @HostBinding text = 'I am in the second row.';
}

@Component({
    selector: "ns-app",
    template: `<GridLayout rows="100, 100, 100", columns="*">
        <Label text="I am in the first row" row="0"></Label>
        <my-cool-component row="1"></my-cool-component>
        <Button text="I am in the third row" row="2"></Button>
    </GridLayout>
    `
})
export class AppComponent { 

}

@darkyelox
Copy link

darkyelox commented Dec 17, 2018

I finally did it, just extends your component class to ContentView class from tns-core-modules/ui/content-view and in the same file of your component (outside and after the class definition) register it using:

registerElement('MyButton', () => require('./my-button.component').MyButtonComponent)

After that your component can use any NativeScript CSS property or component properties like backgroundColor, col and row, etc without the need of wrap it inside another element. 👍

@m-abs
Copy link
Contributor

m-abs commented May 16, 2019

@darkyelox
Great work, thank you very much. It helped us greatly.

One minor thing.
You don't need to extend your component with the ContentView-class, you can just register your component as a ContentView like this:

import { ContentView } from 'tns-core-modules/ui/content-view/content-view';

if (!isKnownView('MyButton')) {
  registerElement('MyButton', () => ContentView);
}

@yassernasc
Copy link

@m-abs you think it can be make automagically on backstage for every new component created?

@danielgek
Copy link

danielgek commented May 16, 2019

@m-abs @yasserN could't this be done inside of Nativescript-angular (i think it's possible), this would be a huge difference because the ui tree would get a lot smaller in some screen implementations and also it would be simpler for beginners.

what do you think @NickIliev ?

@m-abs
Copy link
Contributor

m-abs commented May 16, 2019

It is worth a try.

I've made the changes needed here:
https://github.com/m-abs/nativescript-angular/tree/feat/layoutable-custom-components

I'm concerned this change will break too much.
@NickIliev what do you think?

@danielgek
Copy link

humm indeed Switch from ProxyViewContainer to ContentView might break a few stuff, did you tested an app ?

@m-abs
Copy link
Contributor

m-abs commented May 16, 2019

I've tested in nativescript-sdk-examples-ng and our own app, but not extensively.

It seems to work great and fells smoother but the last part might just be in my head. :)

@yassernasc
Copy link

yassernasc commented May 16, 2019

so, now we will be able to use built-in directives like *ngIf, *ngFor directly on custom components?example: <my-component *ngIf="condition"></my-component>

@VladimirAmiorkov
Copy link
Contributor

Hi @m-abs ,

Are you interested in creating a PR from your fork with the described changes. We could run our tests on that PR to see what the picture is and if something will be broken.

@m-abs
Copy link
Contributor

m-abs commented May 20, 2019

@VladimirAmiorkov Of cause :)
#1832

@vakrilov
Copy link
Contributor

Hey folks,

Components use ProxyViewContainer as their root element because it does not actually render a native view. Actually, ProxyViewContainer was created especially for Angular as it is common for angular application have many levels of nesting of components. Having a dedicated view for each component might lead to performance issue.

Unfortunately, this leads to the the "row/col" issue. It is quite easily solvable by adding a registerElement call in your component. You can specify exactly which layout (or ContentView) to be used as a root-view for the component:

registerElement("my-custom-component", () => { return ContentView });

@Component({
  selector: "my-custom-component",
  // ...
})
export class MyCustomComponent { ... }

You can then use the component and set the row/col properties:

<GridLayout rows="* * *" columns="* * *">
	<my-custom-component row="1" col="1"></my-custom-component>
</GridLayout>

Here is a playground showing this approach.

@m-abs
Copy link
Contributor

m-abs commented May 23, 2019

@vakrilov
I understand the reasoning behind ProxyViewContainer as explained and we can 'just' register the components as ContentView like you suggest.
We've already done so in our project for most components, it removed a lot of extra Layouts.

I think the current behavior is very surprising to most people.I'm sure this isn't the first nor the last issue about the problem.

Without the trick of registering or components as ContentView we ended up with a lot of layouts, which made our app very slow. We've also had to duplicate template code in multiple components, so we could reduce the number of StackLayoutss.

A short while back I reported this this bug on tns-core-modules NativeScript/NativeScript#7226.
This is triggered by nested ProxyViewContainers + *ngIf.

I fully understand not wanting to make ContentView the default view-class in nativescript-angular.
That will also lead to problems and probably break things.

So I agree with the comment by @danielgek on the PR, this should be documented :)

@vakrilov
Copy link
Contributor

vakrilov commented May 23, 2019

I totally agree with you. Here are some suggestions:

  1. We should document the registerElement approach. @NickIliev - can you help with that?
  2. About Crash(iOS): TypeError: undefined is not an object (evaluating 'measureSpec.setColumn') NativeScript#7226 - we were just looking at it with @manoldonev. It look like a bug in ProxyViewContainer - expect a fix.
  3. We are considering the idea that the ProxyViewContainer should also "proxy" the layout specific priorities (like row, col, dock) to its children. It should simplify most of the cases when you just want to position the component in a layout. It will not be possible to implement properties like padding or margin though. They require the container to actually do the layout ... so you might still require to do the registerElement trick you you want those.

Summary: 1. and 2. will be done. 3. - do you think it's a good idea?

@m-abs
Copy link
Contributor

m-abs commented May 23, 2019

Yes, no. 3 is a good idea.

It would still be a bit surprising that styling from the parent component won't work, but I don't think it is needed very much.

Maybe the ProxyViewContainer should give a warning, if the developer attempts to apply padding, margin etc. to it.

@VladimirAmiorkov
Copy link
Contributor

Hi @m-abs ,
I like the idea of a warning in the terminal. @vakrilov we should definitely explore such solution.

@jalberto-ghub
Copy link

Hi @vakrilov
What is the status of this proposal?
Would it include the ability to use *ngIf and *ngFor directly on a custom component?

On the usage of the "registerElement" solution, how should be the right way when working on a shared (WEB/{N}) project? We do not want the WEB to be dependant on {N} stuff. It would be nice if in the {N} version of the module you could declare the components requiring registration and then the core would do it for you auto-magically.

@darkyelox
Copy link

Maybe we can create a typescript decorator over the component that do the work of register it using registerElementand extract the name from the class name or passing it as an argument, something like:

@TNSAngularComponent('SomeNice')
@Component({
    selector: 'SomeNice',
    template: `<StackLayout></StackLayout`
})
export class SomeNiceComponent {
}

@m-abs
Copy link
Contributor

m-abs commented Oct 17, 2019

@darkyelox
I don't think a decorator is all that useful here, especially in AOT builds where we can't get the @Component-params.

You might as well write a normal function to register the component-selector as a TNS View-class.

We use these helper functions.

import { isKnownView, registerElement, ViewClass } from 'nativescript-angular/element-registry';
import { ContentView } from 'tns-core-modules/ui/content-view/content-view';
import { GridLayout } from 'tns-core-modules/ui/layouts/grid-layout/grid-layout';
import { StackLayout } from 'tns-core-modules/ui/layouts/stack-layout/stack-layout';

export function registerComponentView(tagName: string, viewClass: ViewClass = ContentView) {
  if (isKnownView(tagName)) {
    return;
  }

  registerElement(tagName, () => viewClass);
}

export function registerComponentStackLayout(tagName: string) {
  registerComponentView(tagName, StackLayout);
}

export function registerComponentGridLayout(tagName: string) {
  registerComponentView(tagName, GridLayout);
}

@edusperoni
Copy link
Collaborator

@vakrilov I'm weighing in on the option 3 of ProxyViewContainer forwarding layout specific options.

I'm working on a POC that would allow gridlayouts to be specified by using the CSS grid spec. It's really trivial, but it works by watching the direct children's CSS properties and setting their rows/columns accordingly. When we get into angular views, you'd have to wrap them in another layout, which is not ideal for simple components.

On the web, angular views are usually blocks. ProxyViewContainer is supposed to be a helper to decrease view complexity as native views are heavier. I imagine the developer should be aware of this difference, but we should aim to ease this web to native transition.

This should give the developer some additional options when styling their views (especially simple presentational ones):

<GridLayout ...>
  <single-child-component row="1"></single-child-component> <!-- works -->
</GridLayout>
<GridLayout ...>
  <StackLayout row="1">
    <multi-children-component></multi-children-component> <!-- works -->
  <StackLayout>
</GridLayout>
<GridLayout ...>
  <GridLayout row="1" multi-children-component></GridLayout> <!-- directive-like component for even more optimization -->
</GridLayout>

@vakrilov
Copy link
Contributor

I also lean towards option 3. It will improve the usability a lot.
@edusperoni if you are thinking about contributing here is the code of the ProxyViewContaienr and here are how the GridView layout properties are defined.

@m-abs
Copy link
Contributor

m-abs commented Oct 22, 2019

@vakrilov

It would be fairly easy to "proxy" the layout properties directly to top-view of the component, if there is just one.
But what it there is more than one?

For instance:

<Label text="First Text"></Label>
<Label text="Second Text"></Label>

Should it be like with ListView items that are wrapped in a StackLayout if they are not a Layout or is a ProxyViewContainer?
https://github.com/NativeScript/NativeScript/blob/master/nativescript-core/ui/list-view/list-view.android.ts#L315-L329
https://github.com/NativeScript/NativeScript/blob/master/nativescript-core/ui/list-view/list-view.ios.ts#L437-L441

Or should we throw an error like it is done for ContentView.layoutView?
https://github.com/NativeScript/NativeScript/blob/master/nativescript-core/ui/content-view/content-view.ts#L30-L46

I also think it is important, that we don't just focus on the GridLayout properties but support the other layouts too. Like DockLayout and WrapLayout

@m-abs
Copy link
Contributor

m-abs commented Oct 22, 2019

A small Proof-of-Concept (please don't use this in production):

/**
 * Set layout property to the ProxyViewContainers child. Assumes there is only one child.
 */
ProxyViewContainer.prototype['_updateLayoutProperty'] = function(this: ProxyViewContainer, propName: string, value: any, attempt = 0) {
  let first = true;

  const customPropName = `_custom_${propName}`;
  const timeoutPropName = `${customPropName}_timeout`;
  if (this[timeoutPropName]) {
    clearTimeout(this[timeoutPropName]);
  }
  delete this[timeoutPropName];
  this[customPropName] = value;

  this.eachChild((v) => {
    if (first) {
      v[propName] = value;
    }

    first = false;
    return true;
  });

  if (first) {
    // Children not added yet, delay for 1ms and retry.
    if (attempt > 5) {
      return;
    }

    this[timeoutPropName] = setTimeout(() => this['_updateLayoutProperty'](propName, this[customPropName], attempt + 1), 1);
  }
};

ProxyViewContainer.prototype['_getLayoutProperty'] = function(this: ProxyViewContainer, propName: string) {
  let first = true;

  let value: any;

  this.eachChild((v) => {
    if (first) {
      value = v[propName];
    }

    first = false;
    return true;
  });

  if (first) {
    return this[`_custom_${propName}`];
  }

  return value;
};

// Override ProxyViewContainer's layout properties, and set the value on the child view.
const layoutProperties = [
  // AbsoluteLayout
  'left',
  'top',

  // DockLayout
  'dock',

  // FlexLayout
  'flexDirection',
  'flexWrap',
  'justifyContent',
  'alignItems',
  'alignContent',
  'order',
  'flexGrow',
  'flexShrink',
  'flexWrapBefore',
  'alignSelf',
  'flexFlow',
  'flex',

  // GridLayout
  'column',
  'columnSpan',
  'col',
  'colSpan',
  'row',
  'rowSpan',
];

for (const propName of layoutProperties) {
  Object.defineProperty(ProxyViewContainer.prototype, propName, {
    configurable: true,
    set(this: ProxyViewContainer, value: any) {
      this['_updateLayoutProperty'](propName, value);
    },
    get() {
      return this['_getLayoutProperty'](propName);
    },
  });
}

@m-abs
Copy link
Contributor

m-abs commented Oct 23, 2019

I've started working on it here: https://github.com/m-abs/NativeScript/tree/feat/proxy-layout

@vakrilov
Copy link
Contributor

vakrilov commented Oct 24, 2019

Hey @m-abs
Definitely agree on proxy-ing all layout-container properties (not only row and column of the GridLayout).
About what to do when there are multiple children. I would vote for just applying the props to all of them. Because:

  1. Throwing an error seems too harsh (we don't throw an error if you set a row="1" on an element that is not inside a GridLayout for example.)
  2. Creating a StackLayout under the hood will defeat the purpose of ProxyViewContainer- limiting the number of actual layout containers. Better let the developers be explicit and pick their own container.
  3. Applying the same property to all elements will:
    1. Render elements one on top of the other (in case of grid or absolute layout). In this case it should be easy to figure out what to do - probably wrap them in a layout to handle how they are positioned. This is the same behavior if you put multiple elements in a grid and don't assign any rows/cols.
    2. Render them one after another (in case of stack, dock, flex). Which is probably what you would expect them to do.

☝️ Of course this is just my opinion and I would love to hear others too.

One thing that is probably a good idea is to check if any of those properties is already set on the child elements and don't override it in this case. This will allow you to be able to explicitly set properties on the children. This is something that some people already do and it's good to be back-compatible.

@m-abs
Copy link
Contributor

m-abs commented Oct 25, 2019

About what to do when there are multiple children. I would vote for just applying the props to all of them. Because:

I decided to apply them to all children and log it as an error, if there is more than one child.

One thing that is probably a good idea is to check if any of those properties is already set on the child elements and don't override it in this case. This will allow you to be able to explicitly set properties on the children. This is something that some people already do and it's good to be back-compatible.

Good idea.

I'm also looking into how to give a warning, if styling is applied to the ProxyViewContainer directly, but it shouldn't give a warning for values that are inherited from the parent View.

@m-abs
Copy link
Contributor

m-abs commented Nov 27, 2019

Sorry it took so long, but now I've created the PR on for NativeScript core.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests