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

Should first call of view.element render view.template? #262

Closed
oskarwrobel opened this Issue Jul 3, 2017 · 10 comments

Comments

Projects
None yet
3 participants
@oskarwrobel
Member

oskarwrobel commented Jul 3, 2017

This might cause confusing situations.

Using this.element inside view constructor (eg to initialize FocusTracker or KeystrokeHandler) renders the template and makes it not possible to be extended.

class MyView extends View {
    constructor( locale ) {
        super( locale );
         
        this.template = new Template( ... );

        this.focusTracker = new FocusTracker();
        this.fousTracker.add( this.element );
    }
}

const view = new MyView( locale );

Template.extend( view.template, { .... } );

And I see a warning:

template-extend-render: Attempting to extend a template which has already been rendered.

but my view is not even initialized.

To workaround this problem I can move FT init to the view init() but it took me a while to figure out why I can't extend this template.

@oleq

This comment has been minimized.

Member

oleq commented Jul 3, 2017

The very first idea was that constructor defines but it's init does things related to rendering. Also, if not referenced, the view#element does not render, which supposed to be a kind of optimization against unnecessary execution and DOM manipulation.

When we first designed such approach, it looked right. TBH, today, I'm no so sure and it looks to me more like a premature optimization instead. It's confusing in cases like this one. Developers will struggle to understand the magic of view#element and it might not be the best DX.

What if we moved view.element = this.template.render() to View#init? It would mean that each class inheriting from View and implementing own init must first call super.init() in the method definition. I wonder if that would break something major.

WDYT? (cc @Reinmar)

@Reinmar

This comment has been minimized.

Member

Reinmar commented Jul 3, 2017

There reason why we don't render the element in constructor() is to allow subclass existing view to change its template. That's the reason why this.element should not be accessed in the constructor(). Optimisation had nothing to do with that.

I completely agree that this is not the cleanest solution. Perhaps we could replace init() with render() so it would be clearer that until render() is called this.element is null and accessing it changes nothing. Less magic, more clarity. WDYT?

@oleq

This comment has been minimized.

Member

oleq commented Jul 4, 2017

There reason why we don't render the element in constructor() is to allow subclass existing view to change its template. That's the reason why this.element should not be accessed in the constructor(). Optimisation had nothing to do with that.

I know, but instead of saying:

– Don't use #element in constructor if you want to subclass the #template.

we can simply say

view#element is always null in constructor but it is rendered and accessible in init.

We don't need to rename init to render IMO, it doesn't change much in DX.

@Reinmar

This comment has been minimized.

Member

Reinmar commented Jul 4, 2017

We don't need to rename init to render IMO, it doesn't change much in DX.

Depends on the semantics we want to have. What we do in init() now? Isn't this anything more than finishing rendering the view by adding missing listeners or something?

@oleq

This comment has been minimized.

Member

oleq commented Jul 4, 2017

ATM View#init triggers recursive init of children and sets the #ready flag.

Particular classes inheriting from View do tons of stuff, including async initialization (see IframeView, still async after #225). I think it would be weird to write view.render().then( ... ). view.init().then( ... ) feels more precise because many things can happen at this stage and rendering is just one of them.

@Reinmar

This comment has been minimized.

Member

Reinmar commented Jul 4, 2017

I don't think that we need anything besides render(). My idea was to replace the init() method with render() completely. Base View#render() would set view#element and you would normally override it to add more behavior.

@Reinmar

This comment has been minimized.

Member

Reinmar commented Jul 4, 2017

Just look at this piece of docs:

image

Wouldn't render() better fit this domain?

@oleq

This comment has been minimized.

Member

oleq commented Sep 11, 2017

I just fell for that one today. When adding view elements to FocusTracker/FocusCycler I didn't realise their templates are extended later on and I accidentally rendered them too early and broke some stuff added by the Template.

I think we should consider dropping this auto-rendering in the View#element getter. Although it warns in the console, it may be very hard to understand for the outside world anyway.

@Reinmar

This comment has been minimized.

Member

Reinmar commented Sep 12, 2017

I marked this as 1.0.0 candidate.

@Reinmar

This comment has been minimized.

Member

Reinmar commented Oct 10, 2017

Can lead to the removal of the init() method. All custom stuff will happen in render().

class MyView extends View {
     render() {
          const el = super.render();
         
          // Custom stuff.

          return el;

          // But what about `this.element`? Maybe returning makes no sense.
    }
}

@Reinmar Reinmar referenced this issue Oct 12, 2017

Closed

[Umbrella] UI improvements #323

8 of 18 tasks complete

@oleq oleq self-assigned this Oct 18, 2017

@oleq oleq added this to the iteration 13 milestone Oct 18, 2017

oskarwrobel added a commit that referenced this issue Nov 2, 2017

Merge pull request #332 from ckeditor/t/262
Other: Implemented `View#render` method which replaces the `#element` rendering upon the first reference and incorporates the `#init` method functionality. Closes #262. Closes #302.

From now on `View#setTemplate` and `View#extendTemplate` methods are recommended as a shorthand for `view.template = new Template( { ... } )` and `Template.extend( view.template )`.

BREAKING CHANGE: The `init()` method in various UI components has been renamed to `render()`. Please refer to the documentation to learn more.

BREAKING CHANGE: The `View#element` is no longer a getter which renders an element when first referenced. Use the `View#render()` method instead.

BREAKING CHANGE: `Template#children` property became an `Array` (previously `ViewCollection`).

BREAKING CHANGE: `View#addChildren` and `View#removeChildren` methods became `#registerChildren` and `#deregisterChildren`.

BREAKING CHANGE:  The DOM structure of the `StickyPanelView` has changed along with the class names. Please refer to the component documentation to learn more.

oskarwrobel added a commit to ckeditor/ckeditor5-editor-balloon that referenced this issue Nov 2, 2017

Tests: Simplified BalloonEditorUI tests.
Internal: Aligned the UI to the latest API of the framework (see ckeditor/ckeditor5-ui#262).

oskarwrobel added a commit to ckeditor/ckeditor5-core that referenced this issue Nov 2, 2017

Merge branch 'master' into t/ckeditor5-ui/262
Internal: Aligned the UI to the latest API of the framework (see ckeditor/ckeditor5-ui#262).

oskarwrobel added a commit to ckeditor/ckeditor5-editor-inline that referenced this issue Nov 2, 2017

Code refactorin in the editor-inline package.
Internal: Aligned the UI to the latest API of the framework (see ckeditor/ckeditor5-ui#262).

oskarwrobel added a commit to ckeditor/ckeditor5-editor-classic that referenced this issue Nov 2, 2017

Code refactorin in the editor-classic package.
Internal: Aligned the UI to the latest API of the framework (see ckeditor/ckeditor5-ui#262).

oskarwrobel added a commit to ckeditor/ckeditor5-link that referenced this issue Nov 2, 2017

Aligned link package to the View#render API.
Internal: Aligned the UI to the latest API of the framework (see ckeditor/ckeditor5-ui#262).

oskarwrobel added a commit to ckeditor/ckeditor5-heading that referenced this issue Nov 2, 2017

Aligned heading package to the View#render API.
Internal: Aligned the UI to the latest API of the framework (see ckeditor/ckeditor5-ui#262).

oskarwrobel added a commit to ckeditor/ckeditor5-theme-lark that referenced this issue Nov 2, 2017

Tests: Updated theme manual test to the View#render API.
Internal: Aligned the UI to the latest API of the framework (see ckeditor/ckeditor5-ui#262).

oskarwrobel added a commit to ckeditor/ckeditor5-image that referenced this issue Nov 2, 2017

Aligned the Image package to the View#render API.
Internal: Aligned the UI to the latest API of the framework (see ckeditor/ckeditor5-ui#262).

oskarwrobel added a commit to ckeditor/ckeditor5-upload that referenced this issue Nov 2, 2017

Aligned the upload package to the View#render API.
Internal: Aligned the UI to the latest API of the framework (see ckeditor/ckeditor5-ui#262).

oskarwrobel added a commit to ckeditor/ckeditor5-utils that referenced this issue Nov 2, 2017

Docs: Fixed wrong isDomNode utility module's name.
Internal: Aligned the UI to the latest API of the framework (see ckeditor/ckeditor5-ui#262).

This was referenced Nov 14, 2017

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