lineupjs icon indicating copy to clipboard operation
lineupjs copied to clipboard

Extension of Dialogs is not possible due to design and missing exports

Open puehringer opened this issue 4 years ago • 4 comments

  • Release number or git hash: v4.0.0-beta.1

Observed behavior

  • Example: If I want to customize the StringFilterDialog to not show the Regular Expression checkbox, I have to copy the entire dialog and remove the checkbox "manually".
    • Copying is problematic because it duplicates code
    • Copying is barely possible because many internal utility functions are used in the dialogs which should not be exported
    • Inheritance is not possible because:
      • the dialogs are not exported
      • the dialogs do not offer functions such as show|hideRegexCheckbox (which is fine, but should be implementable in the subclasses)
      • the relevant inputs are usually accessed by a querySelector, such that subclasses cannot access this.regexCheckbox without the chance of breaking in the future

Expected behavior

  • Example: I want to have the StringFilterDialog without the Regular Expression checkbox (should be checked by default) and without the Filter missing checkbox (should be checked if the filter itself is not empty).

The following is an example how it could look like:

export default class SimpleStringFilterDialog extends StringFilterDialog {

  protected updateFilter(filter: string | RegExp | null, filterMissing: boolean) {
    // Set the filterMissing to true if filter is truthy
    super.updateFilter(filter, Boolean(filter));
  }

  protected build(node: HTMLElement) {
    super.build(node);
    // Hide the filterMissingCheckbox and regexCheckbox checkboxes
    // Note: currently, they might refer to the actual checkbox and not to the "container" with the label. Maybe add filterMissingCheckbox[Container] to this?
    this.filterMissingCheckbox.style.display = 'none';
    this.regexCheckbox.style.display = 'none';
    this.regexCheckbox.checked = true;
  }
}

In reality this is not possible because:

  • StringFilterDialog and all other dialogs are not exported
    • Solution: Export it in own namespace dialogClasses and dialogUtils like the renderers (https://github.com/lineupjs/lineupjs/pull/207)
  • updateFilter (or the equivalent in other dialogs) is private
    • Solution: Changing it to protected should not cause any issues
  • this.filterMissingCheckbox and this.regexCheckbox do not exist yet, as they are currently accessed via this.findInput('input[type="checkbox"]') or the internal utility function findFilterMissing(this.node), making it nearly impossible to access them in a clean way in subclasses.
    • Solution: Create protected members for the important UI elements of a dialog to allow the subclasses to modify them.
  • Alternatively, it would be much nicer to have functions such as setRegexCheckbox(boolean) or even allowing options to be passed in the constructor, but that would assume that people often use these dialog APIs, besides the massive refactoring. I think it is better to allow developers to access the internals in a safe and easy way, such that they can implement it themselves.

This is just a simple example, but it should generalize to all other dialogs as well. I would appreciate any input/discussion about a better way to accomplish the task(s) above.

puehringer avatar Jun 08 '20 06:06 puehringer

Related to this is my change request in https://github.com/lineupjs/lineupjs/pull/334#issuecomment-639560277 point 2.

thinkh avatar Jun 08 '20 07:06 thinkh

why not just implement the whole dialog in this case as in https://codepen.io/sgratzl/pen/XWXmWQR?editors=1010

sgratzl avatar Jun 08 '20 07:06 sgratzl

@sgratzl That's exactly how it was implemented prior to LineUp4. But with the update to LineUp4 every dialog broke because of the new functions such as cancel and reset. Therefore, we had to update all the dialogs although they only change minor things (checkbox hiding, default checking, ...). With the ability to extend existing dialogs, we will not have this problem in the future, and we will always be in sync (except for minor updates on a per-dialog basis). I understand that it has to be carefully decided if a dialog is supposed to be a completely new dialog, or just the extension of an existing one. But the main use case, the minor adjustment of a dialog, would be easily possible with the above solution. Another example would be the hiding of the Filter Missing checkbox in the CategoricalFilterDialog. Reimplementing it from scratch is rather difficult, as many dependencies are also marked internal such as https://github.com/lineupjs/lineupjs/blob/lineup-v4/src/model/internalCategorical.ts#L174-L193. Even staying in sync with the styles is difficult, as cssClass is also internal. One can not be expected to copy all dependencies in your own codebase, as this will cause many problems in the future by being out of sync, etc. The point is that we want to do minor adjustments, which are not worth the trouble (and problems...) of reimplementing an entire dialog including sort/filter/rendering logic.

puehringer avatar Jun 08 '20 07:06 puehringer

@thinkh so what is the plan?

sgratzl avatar Jun 17 '20 08:06 sgratzl