-
Notifications
You must be signed in to change notification settings - Fork 61
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
Hide hidden columns from trace viewer #179
base: master
Are you sure you want to change the base?
Hide hidden columns from trace viewer #179
Conversation
Use the "type" parameter to encode hidden columns. Signed-off-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
@@ -159,5 +171,6 @@ export class TableOutputComponent extends AbstractOutputComponent<TableOutputPro | |||
this.setState({ | |||
tableColumns: this.columnArray | |||
}); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do I understand correctly that the type of the column header's type field should be an object? In the TSP, it is now a string. While this change probably works fine, iiuc, it won't match the TSP's specification anymore? We have to be careful to fix the TSP very soon, to avoid the trace server answering to this specific client and bypassing the TSP. Or am I wrong in my understanding of the patch?
const typeString: string | null = columnHeader.type; | ||
let hiddenColumn = false; | ||
if (typeString) { | ||
const type: ColumnHeaderType = JSON.parse(typeString); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the column type field will need to be better documented in the tsp to mention those header types. But I guess that will be part of a more complete overhaul of the data type definition that we discussed a few weeks ago. Right now, the type is a string, while this code suggests it should be an object.
const id = this.showIndexColumn ? ++columnHeader.id : columnHeader.id; | ||
colIds.push(id); | ||
columnsArray.push({ | ||
headerName: columnHeader.name, | ||
field: columnHeader.id.toString(), | ||
width: this.props.columnWidth | ||
width: this.props.columnWidth, | ||
hide: hiddenColumn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the ag-grid, the hide property accepts boolean.
https://www.ag-grid.com/javascript-grid-column-properties/
In our case, the hidden column is set to string after JSON.parse. It's good to set to boolean though it works in this case.
let hiddenColumn = false; | ||
if (typeString) { | ||
const type: ColumnHeaderType = JSON.parse(typeString); | ||
hiddenColumn = type.hidden; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check for undefined when the 'hidden' property doesn't exist.
@MatthewKhouzam if the column is hidden, is there a way to show them in the front-end with the current implementation? |
@MatthewKhouzam Is this PR still valid? |
@MatthewKhouzam, the column model in the TSP doesn't have the hidden flag at the moment. If we'd like to support to allow the backend to hide columns by default this would have to be added to the TSP, trace server and front-end. Shall this one be issue be closed for now. We can work on it when it becomes priority? |
Use the "type" parameter to encode hidden columns.
Signed-off-by: Matthew Khouzam matthew.khouzam@ericsson.com