lineupjs
lineupjs copied to clipboard
Extension of Dialogs is not possible due to design and missing exports
- 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
anddialogUtils
like the renderers (https://github.com/lineupjs/lineupjs/pull/207)
- Solution: Export it in own namespace
-
updateFilter
(or the equivalent in other dialogs) is private- Solution: Changing it to protected should not cause any issues
-
this.filterMissingCheckbox
andthis.regexCheckbox
do not exist yet, as they are currently accessed viathis.findInput('input[type="checkbox"]')
or the internal utility functionfindFilterMissing(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.
Related to this is my change request in https://github.com/lineupjs/lineupjs/pull/334#issuecomment-639560277 point 2.
why not just implement the whole dialog in this case as in https://codepen.io/sgratzl/pen/XWXmWQR?editors=1010
@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.
@thinkh so what is the plan?