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

Table: add TableOrganizer and ShowInvisibleColumnsForm for Scout JS #980

Open
wants to merge 8 commits into
base: releases/24.2
Choose a base branch
from

Conversation

bschwarzent
Copy link
Member

No description provided.

@bschwarzent bschwarzent self-assigned this Apr 12, 2024
@bschwarzent bschwarzent force-pushed the features/bschwarzent/24.2/369108_show_invisible_columns_1 branch from 9916294 to 4283ad2 Compare April 18, 2024 14:31
- Use scout.assertParameter() instead of custom Error
- Use 'page' as parameter name instead of 'node'
- Adjust return type from 'Form | Table' to 'OutlineContent'
- Extract method
- Table/Tree implements Filterable
- TableRow/TreeNode implements FilterElement
- LazyNodeFilter implements Filter
- dates.formatTimestamp(): format a date in a fixed timestamp format
- numbers.randomInt(): generate a random integer
The initial location has no explicit route, but this is not an error.
Because the URL is not only used for routing (e.g. we can have query
params), we cannot reliably determine when to emit a warning. To avoid
unnecessary log entries, the log level of the warning was reduced to
info.
objects.checkFunctionOverrides() was used in the pre-ES6 era to check
in the F12 developer console if the signature of "overwritten" methods
in the prototype chain matches. This utility no longer works correctly
and is no longer needed (we now rely on ES6/TS classes and IDE support).
- When a tree node is selected, ensureExpansionVisible() is called to
  make sure the expanded children of this node are within the viewport.
- This method should only consider nodes that are actually visible, i.e.
  not filtered (e.g. by lazy expansion).
- If a node has no children, at least the node itself should be scrolled
  inside the viewport.
- Because the outline tree has no horizontal scrollbar, no scrolling
  should be performed in that direction (even when the tree nodes are
  pushed out of the visible area).

336993
The TableHeaderMenu consults the table whether a column can be added,
removed or modified. In the case of Scout Classic, this depends on some
static properties computed by the UI server (e.g. "columnAddable").
In Scout JS, these properties are now ignored. Instead, the decision is
delegated to a TableOrganizer instance, which is created for every
table. The organizer handles the 'columnOrganizeAction' menu which is
triggered by the buttons in the table header menu. The basic
implementation can change the 'visible' flag of existing columns. In
the future, it will be possible to create new columns via a separate
TableCustomizer (not yet implemented).

369108
@bschwarzent bschwarzent force-pushed the features/bschwarzent/24.2/369108_show_invisible_columns_1 branch from 4283ad2 to 5897e34 Compare April 19, 2024 15:52
@@ -100,7 +89,7 @@ protected int getConfiguredGridColumnCount() {
@Override
protected void execInitField() {
super.execInitField();
setStatusVisible(false);
//setStatusVisible(false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this no longer required? Then remove the line? Otherwise remove comment?

@@ -268,4 +268,5 @@ export interface TableModel extends WidgetModel {
*/
textFilterEnabled?: boolean;
defaultMenuTypes?: string[];
tableOrganizer?: ObjectOrModel<TableOrganizer>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if a tableOrganizer model is passed as part of this TableModel, is it actually used when creating the instance?
Either it should have an effect or here only a TableOrganizier can be passed and no model?

objects.replacePrototypeFunction(Table, '_createTableOrganizer', function(this: Table & { _createTableOrganizerOrig }) {
if (this.modelAdapter) {
// nop in classic mode
return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe return null here instead of undefined? But actually makes no difference

columns: [
{
id: 'KeyColumn',
objectType: Column,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specify the generic here (Column)? WidgetMap can handle generics now.

this.insertAfterColumn = null;
}

protected override _jsonModel(): WidgetModel {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return type could be a FormModel

if (this.table.tableOrganizer) {
return this.table.tableOrganizer.getAddableColumns(this.insertAfterColumn);
}
return this.table.displayableColumns()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this case actually happen? This Form is only called from the Organizer, right? Then it must be present? Or not?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The table organizer is null in classic mode (method _createTableOrganizer is replaced in TableAdapter)

this.widget('ColumnsTable').insertRows(rows);
}

protected _getAddableColumns(): Column[] {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The list of available columns is computed by the tableOrganizer. Maybe instead of get them here, the organizer could pass it in the model for this form when creating it?

return {
cells: [
scout.create(Cell, {value: column}),
this._getColumnTitle(column)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Scout Classic if the tooltip is used as column title, it is displayed using italic font. Do we want this here as well? Or not?

* Returns true if the given column can be modified.
*/
isColumnModifiable(column: Column): boolean {
if (this.table.isCustomizable()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

directly return this.table.isCustomizable()

protected _removeColumn(column: Column) {
column.setVisible(false);

// TODO bsh [js-table] Should we keep the sorting? In Java, it stays intact (only visible in the organize menu).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe keep it the same as in java? Or is this expected by the user if a column is removed that the rows change in order?

@mvilliger
Copy link
Member

mvilliger commented Apr 23, 2024

Table grouping seems not to work correctly. E.g. when grouping by column A and removing column B, the whole grouping seems to be removed. This seems not to be correct?
Furthermore if grouping for multiple columns is configured and one of these is removed, the whole grouping is removed as well?

@mvilliger
Copy link
Member

Another issue:

  1. Add an aggregation for a number column (e.g. avg).
  2. Remove this number column -> the aggregation row is removed correctly
  3. Add the aggregation column again -> aggregation row is not shown, but if the column header is clicked it still shows as active

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

Successfully merging this pull request may close these issues.

None yet

2 participants