ngx-inline-editor icon indicating copy to clipboard operation
ngx-inline-editor copied to clipboard

How should the public API be? We want your feedback

Open tonivj5 opened this issue 7 years ago β€’ 18 comments

With the last PR (#57), the public API is going to change and it would be good opinions and way of to do this.

At the moment, the public API is something like this: All events return an ExternalEvent (exported as InlineEvent) ->

{ 
  event?: Event, 
  state: { value: any, empty: boolean,  editing: boolean, disabled: boolean } // it's an InlineState
}

An state is the state of input when this one changed (the new state), for example: if I click an input, its state will change from editing: false to editing: true, and if I type 'a', its state will change from value: '' to value: 'a'. These states are in hot, until I do click on save, this changes will not reflect in the model (ngModel) and I will receive it using the events ((onChange)="todoSomething($event)", etc.).

  • Events bindable
    • onSave
    • onEdit
    • onCancel
    • onError // This one returns an InlineError | InlineError[] -> { type: string, message: string }
    • onChange
    • onKeyPress
    • onEnter
    • onEscape
    • onFocus
    • onBlur
  • Public methods (API) (with these methods you can change the state of input)
    • save( { event: Event, state: InlineState }: ExternalEvent )
    • saveAndClose( { event: Event, state: InlineState }: ExternalEvent )
    • edit( { editing: boolean, doFocus: boolean } )
    • cancel( { event: Event, state: InlineState }: ExternalEvent )
    • getHotState(): InlineState // This returns state from input (no saved)

Are the methods user friendly? is this the correct approach? Please, give us your feedback πŸ‘

tonivj5 avatar May 16 '17 07:05 tonivj5

Very excited to see these enhancements, @xxxtonixxx! The new inputs and event bindings are extremely useful.

We are trying to understand how to invoke methods in the API but I don't see any examples to follow nor documentation of the API. How does one get a reference to a given <inline-editor> instance to invoke the public methods?

KeithGillette avatar Jun 09 '17 13:06 KeithGillette

@KeithGillette you would use the events the following way:

<inline-editor (onSave)="yourHandleSave">
<inline-editor (onError)="yourHandleError">
<inline-editor (onChange)="yourHandleChange">

Caballerog avatar Jun 09 '17 15:06 Caballerog

Thanks for the fast response, @Caballerog. I understand how to invoke my methods upon a given event from the inline-editor as you have shown. My question is how to put a given <inline-editor> instance into a given state using the public API methods (save(), saveAndClose(), edit(), cancel(), getHotState()) referenced above.

KeithGillette avatar Jun 09 '17 16:06 KeithGillette

Thanks @KeithGillette for your interest! πŸ˜„

The actual documentation (the README) is outdated, so in this moment there is not any documentation or examples... It is the next step we want to do!

If you want to get an instance of InlineEditorComponent you should use the @ViewChild() decorator or defining the reference into the template and pass it like a function parameter (ex: <inline-editor #myInlineEditor (onBlur)="doSomethingUsingPublicAPI(myInlineEditor)"></inline-editor>

Here you have a complete code-example:

import { Component, ViewChild, AfterViewInit } from '@angular/core';
import { InlineEditorComponent, InlineEditorEvent } from '@qontu/ngx-inline-editor';
import { InputTextConfig } from '@qontu/ngx-inline-editor/configs';

@Component({
  selector: 'app-root',
  template: `
  <inline-editor #myInlineEditorInstance [(ngModel)]="test" [config]="config"></inline-editor>
  <inline-editor #myInlineEditorRefFromTemplate [(ngModel)]="test" [config]="config"></inline-editor>
  
  <button (click)="myInlineEditorRefFromTemplate.edit({editing: true, event: $event})">
    Edit the 2ΒΊ inline-editor without create the property into class
  </button>

  <button (click)="myInlineEditorRefFromTemplate.cancel()">
    Remove  the editable state from the 2ΒΊ inline-editor without create the property into class
  </button>

  <button (click)="logg(myInlineEditorRefFromTemplate)">
    Console.log of inline-editor's value passing editor by parameter to function
  </button>
  `,
})
export class AppComponent implements AfterViewInit {

  test = 'hi';
  
  // The input instance, it is available on AfterViewInit hook
  @ViewChild('myInlineEditorInstance') myInlineEditorInstance: InlineEditorComponent;

  config: InputTextConfig = {
    type: 'text',
  };

  ngAfterViewInit(): void {
    // the input became to editable and focused when the page load
    setTimeout(
      () => this.myInlineEditorInstance.edit({ editing: true, doFocus: true }),
    );
  }

  logg(inlineEditor: InlineEditorComponent) {
    const { value } = inlineEditor.state.getState();
    console.log(`This is the value of inline-editor getting the inline-editor instance from parameter: ${value}`);
  }

}

We would appreciate a lot your feedback! πŸ‘

tonivj5 avatar Jun 09 '17 17:06 tonivj5

This is outstanding. Thanks for the fast response and detailed example, @xxxtonixxx!

My use-case is a bit more complex, but I think I just figured out a solution.

Within the template for my component, we have dynamically generated InlineEditorComponent instances (inside of a treeview). I can dynamically assign each a unique id in the name property: <inline-editor [name]="node.id">, where node.id is a uniquely defined value within the parent element for each InlineEditorComponent instance. I can then also access an array of all InlineEditorComponents in the populated template in my controller:

@ViewChildren(InlineEditorComponent) private inlineEditorViewChildren: QueryList<InlineEditorComponent>;

In my methods, I can then perform a find on the QueryList to identify the correct InlineEditorComponent on which to invoke the public API calls:

public editNode(nodeId): void {
  const inlineEditorToInvoke = 
    this.inlineEditorViewChildren.find((inlineEditorComponent: InlineEditorComponent) => {
      return inlineEditorComponent.name === nodeId;
    });
inlineEditorToInvoke.edit({editing: true, doFocus: true});
}

KeithGillette avatar Jun 09 '17 20:06 KeithGillette

First, thank you, @xxxtonixxx and @Caballerog for this useful component and these recent API enhancements which have made it much more versatile.

Since you're requesting feedback on these changes, here are a couple of suggestions that have occurred to me in using the new API in my project:

Select on Edit

Add select boolean parameter to the edit method to select the value of the input:

.edit( { editing: boolean, focus: boolean, select: boolean } )

(I know the current property is doFocus, but I think just focus might be preferable. If you keep doSelect, then the new property should be called doSelect for consistency.)

Pass ExternalEvent on all event bindings

In my testing with the text inline editor, the bindable events pass different arguments to the bound methods. onEdit, onSave, onBlur, onEnter, onKeyPress, onChange pass the input value while onCancel passes the ExternalEvent object. The latter is much more useful, so I recommend using it as the argument passed for all event bindings. (With the original event, I could call select() on the srcElement in an onEdit handler, obviating the need for the first suggested change.)

Thanks for considering!

KeithGillette avatar Jun 12 '17 14:06 KeithGillette

By order:

  • Select on edit is a so good option to add and I think focus/select is better too πŸ‘
  • Really, you have discovered a bug (it should call the emit method, which returns the value or object ) πŸ˜… We considered that approach, so we added a config option (onlyValue) and its value is true by default to maintain compatibility with the actual API. Here is used. If you set onlyValue to false, all events emit the ExternalEvent object πŸ˜‰

And thanks a lot for your feedback, it make me so happy! πŸ˜„

tonivj5 avatar Jun 12 '17 15:06 tonivj5

Hi @KeithGillette @xxxtonixxx

I have added the select feature and fixed bug in cancel method https://github.com/qontu/ngx-inline-editor/pull/65 https://github.com/qontu/ngx-inline-editor/pull/66

jesussegado avatar Jun 13 '17 16:06 jesussegado

Another bit of feedback on this extremely useful component:

I have discovered that one can pass additional arguments to the controller methods invoked by the public API from the template bindings, such as (onEnter)=onInlineEditorEnter($event, node) where node is some locally defined template variable. I am not sure whether this behavior is by design or accident, but I have found it critical to have when dealing with automatically generated inline-editor instances embedded in a component, so please keep it!

KeithGillette avatar Jun 21 '17 11:06 KeithGillette

Hi! @KeithGillette, yes, it's an expected behavior and I've written some examples doing something similar.

However, we should add some information about inline-editor's instance which is executing the method, based on this PR https://github.com/qontu/ngx-inline-editor/pull/72

Thanks for your feedback again, we appreciate them a lot πŸ˜„

tonivj5 avatar Jun 21 '17 14:06 tonivj5

Great! So here's another piece of feedback:

I think calling manually edit on an inline-editor instance should override the disabled=true input binding. Here is the use case:

I have a dynamically generated set of inline-editor instances repeated in my component template. I want the inline-editor to be invoked by selecting Rename from a drop-down menu associated with each instance instead of by a click on the inline-editor instance, so I set <inline-editor disabled=true> in the template and invoke the appropriate inline-editor instance via the .edit public API method as described in a previous post.

However, to make this work, I have to also set inlineEditorToInvoke.disabled=false; before inlineEditorToInvoke.edit({editing: true, doFocus: true});. But then I have to find a way to set inlineEditorToInvoke.disabled=true; after editing save or cancel in order to prevent editing via click, which seems overly complicated.

KeithGillette avatar Jun 21 '17 14:06 KeithGillette

@KeithGillette , I think what you need to use is the [editOnClick] option. It is true by default, if you set it to false when you click on inline-editor, it will do nothing. So, you don't need to enable and to disable it on each edition

Is it what you need? πŸ‘

tonivj5 avatar Jun 22 '17 20:06 tonivj5

Yes, thanks, that is indeed exactly what I need, @xxxtonixxx! So please amend my last feedback post to this: The public API should be, well, public, since I think your last reply is the only place [editOnClick] is presently publicly documented. πŸ˜†

KeithGillette avatar Jun 23 '17 00:06 KeithGillette

Hi @KeithGillette I have created a new PR with the docs πŸ˜„ Can you give us some feedback? https://github.com/qontu/ngx-inline-editor/pull/79 it's in WIP too, but that is the way πŸ˜‰

tonivj5 avatar Jul 19 '17 18:07 tonivj5

Hi @xxxtonixxx. Apologies on the delay in responding. I've just returned from vacation this week.

The new documentation site looks great! As you say, it's a work-in-progress, as it's lacking coverage of the bindable events and methods from the new public API.

FYI: I noticed a few small wording changes that might make the documentation clearer but when I clicked the Edit page links, it got a 404 Not Found error.

KeithGillette avatar Aug 01 '17 21:08 KeithGillette

Thanks @KeithGillette for your review!! πŸ˜„

Yeah, I hope to have it ready soon πŸ˜… And thanks for your interest in improve the docs, it fails because the link points to qontu/ngx-inline-editor but it is in my fork, the Edit page link won't work until the PR is merged πŸ‘

tonivj5 avatar Aug 02 '17 21:08 tonivj5

I'm on to my next use case for this component and back with another recommendation for the API:

The onKeyPress binding should fire for all keypresses, not just printable characters. Or better yet, the component should provide an additional configuration property similar to the angular-tree-component KeyboardEvent action mapping allowing custom callbacks to be registered for any keystroke.

This would allow for the creation of a very fluent keyboard-driven user experience. In my case, I have a TreeView of InlineEditorComponents and want to propagate up/down arrow presses to the parent TreeViewNode so that even when the InlineEditor is active, the user can navigate the tree.

KeithGillette avatar Oct 11 '17 18:10 KeithGillette

Hi @KeithGillette, sorry for my delay. Could you open a new issue with your request?

Thanks for your feedback and improvements! :+1:

tonivj5 avatar Oct 30 '17 11:10 tonivj5