Use UI toolkit for variable table #291
Conversation
|
|
There was a problem hiding this comment.
Thanks @kentarolim10
Here are some suggestions. And in addition to those there is one point that needs to be addressed:
Could you please add the following statement in the variableinspector plugin (in index.ts):
addJupyterLabThemeChangeListener();This will link the toolkit theme with the JupyterLab theme.
src/variableinspector.ts
Outdated
| Search, | ||
| Button | ||
| } from '@jupyter/web-components'; | ||
| provideJupyterDesignSystem().register(allComponents); |
There was a problem hiding this comment.
You should import the jp... variant of the components.
Then could import as fine grain the proper components to optimize what is loaded:
| provideJupyterDesignSystem().register(allComponents); | |
| provideJupyterDesignSystem().register(jpDataGrid(), jpDataGridRow(), ...); |
src/variableinspector.ts
Outdated
| export function createCellTemplate(table: WebDataGrid): HTMLTemplateElement { | ||
| const template = document.createElement('template'); | ||
| const container = document.createElement('div'); | ||
| container.innerText = 'testing'; | ||
| template.appendChild(container); | ||
| table.appendChild(template); | ||
| return template; | ||
| } | ||
|
|
There was a problem hiding this comment.
Dead code?
| export function createCellTemplate(table: WebDataGrid): HTMLTemplateElement { | |
| const template = document.createElement('template'); | |
| const container = document.createElement('div'); | |
| container.innerText = 'testing'; | |
| template.appendChild(container); | |
| table.appendChild(template); | |
| return template; | |
| } |
| @@ -52,6 +52,7 @@ | |||
|
|
|||
| .filter-search-container { | |||
| display: flex; | |||
| align-items: center; | |||
| padding: 0 1rem; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
The rule .filter-type should be removed
src/variableinspector.ts
Outdated
| ico.onclick = (ev: MouseEvent): any => { | ||
| console.log('Click on ' + name); | ||
| console.log('Click on ' + item.varName); |
There was a problem hiding this comment.
Let's remove this
| console.log('Click on ' + item.varName); |
|
|
||
| // Add onclick event for inspection | ||
| cell = row.insertCell(1); | ||
| cell = document.createElement('jp-data-grid-cell') as DataGridCell; | ||
| if (item.isMatrix) { | ||
| cell.title = 'View Contents'; | ||
| cell.className = 'jp-VarInspector-inspectButton'; | ||
| const ico = searchIcon.element(); |
There was a problem hiding this comment.
Would you mind using a jp-button to wrap the icon in it (you can use the appearance stealth)?
| cell.title = 'Delete Variable'; | ||
| cell.className = 'jp-VarInspector-deleteButton'; | ||
| cell.gridColumn = '1'; | ||
| const ico = closeIcon.element(); |
There was a problem hiding this comment.
Could you wrap this icon in a jp-button with appearance stealth?
src/variableinspector.ts
Outdated
| import wildcardMatch from 'wildcard-match'; | ||
|
|
||
| const TITLE_CLASS = 'jp-VarInspector-title'; | ||
| const PANEL_CLASS = 'jp-VarInspector'; | ||
| const TABLE_CLASS = 'jp-VarInspector-table'; | ||
| const TABLE_BODY_CLASS = 'jp-VarInspector-content'; | ||
| // const TABLE_BODY_CLASS = 'jp-VarInspector-content'; |
There was a problem hiding this comment.
Dead code?
| // const TABLE_BODY_CLASS = 'jp-VarInspector-content'; |
src/variableinspector.ts
Outdated
| nameOption.value = 'name'; | ||
| nameOption.innerHTML = 'Name'; | ||
| filterType.appendChild(varTypeOption); | ||
| filterType.appendChild(nameOption); | ||
| const searchContainer = document.createElement('div'); | ||
| searchContainer.className = 'jp-InputGroup filter-search-container'; | ||
| const input = document.createElement('input'); | ||
| const input = document.createElement('jp-search') as Search; |
There was a problem hiding this comment.
The jp-text-field will be more appropriate than the search component.
|
@fcollonval I have made the request changes to the PR 👍 |
Description
Old styling
Pr for #277