KDS-547: Tokenization of Python scripting – Temporary Values Panel #39
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements tokenization updates for the Python scripting Temporary Values Panel, migrating from legacy KNIME component styles to the KNIME Design System (KDS). The changes focus on updating CSS styling to use KDS design tokens and replacing the old Button component with KdsButton.
- Migrated CSS from hardcoded values to KDS design tokens for consistent theming
- Replaced
@knime/componentsButton with@knime/kds-componentsKdsButton - Updated table header/body styling to follow KDS spacing and visual patterns
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| PythonWorkspaceHeader.vue | Updated table header styling with KDS tokens, changed sticky positioning structure, added KDS font and color tokens |
| PythonWorkspaceBody.vue | Applied KDS spacing, typography, and hover state styling to table rows |
| PythonWorkspace.vue | Migrated Button to KdsButton, replaced hardcoded spacing/colors with KDS tokens throughout |
| package.json | Added @knime/kds-components dependency and downgraded @vueuse/core |
org.knime.python3.scripting.nodes/js-src/src/components/PythonWorkspaceHeader.vue
Outdated
Show resolved
Hide resolved
org.knime.python3.scripting.nodes/js-src/src/components/PythonWorkspaceBody.vue
Outdated
Show resolved
Hide resolved
| :disabled="!resetButtonEnabled" | ||
| @click="resetWorkspace" | ||
| > | ||
| Reset values |
There was a problem hiding this comment.
The button text 'Reset values' is duplicated between the 'label' prop (line 80) and the slot content. Remove the slot content since the label prop already provides the button text.
There was a problem hiding this comment.
No slots in KdsButtons anymore!
senemdilli
force-pushed
the
KDS-547-python-scripting-temp-values
branch
from
cf613ca to
a7bea4a
Compare
senemdilli
force-pushed
the
KDS-547-python-scripting-temp-values
branch
from
a7bea4a to
ef340ba
Compare
senemdilli
force-pushed
the
KDS-547-python-scripting-temp-values
branch
2 times, most recently
from
c2d35a6 to
8a70956
Compare
senemdilli
force-pushed
the
KDS-547-python-scripting-temp-values
branch
from
8a70956 to
8e86d30
Compare
senemdilli
force-pushed
the
KDS-547-python-scripting-temp-values
branch
from
8e86d30 to
08ccdb4
Compare
| const resizeContainer = ref<HTMLElement | null>(null); | ||
| const totalWidth: Ref<number> = ref(0); | ||
| const headerWidths = ref<ColumnSizes>([100, 100, 100]); | ||
| const paddingAroundTable = 8; |
There was a problem hiding this comment.
Magic number 8 is used to calculate totalWidth but doesn't correspond to the actual padding values applied (--kds-spacing-container-0-5x on lines 106-107). This hardcoded value should either use the same KDS token value or be calculated from the actual applied padding to prevent layout inconsistencies.
There was a problem hiding this comment.
@ChristianAlbrecht, we already have the tokens in JSON, right? Is it somehow exported to be accessed in JS?
There was a problem hiding this comment.
Yes, the tokens are saved in a standardized JSON format, but that is not supposed to be consumed.
For JS/TS we can output a second build artifact which exposes final tokens as JS variables. I did a couple of experiments on this already and it should easily be possible, but currently we don't have this output yet.
I wanted to enable it only for the canvas based workflow editor, but if we need it earlier we can also move it up.
However since it's only 1 value here (as far as I have seen) I would say let's leave it as a magic number or try to solve it completely in CSS if possible. Not sure on the specifics why this size calculation was needed in JS.
There was a problem hiding this comment.
If it needs to stay in JS just add a TODO comment to replace it once the JS tokens are available.
| const resizeContainer = ref<HTMLElement | null>(null); | ||
| const totalWidth: Ref<number> = ref(0); | ||
| const headerWidths = ref<ColumnSizes>([100, 100, 100]); | ||
| const paddingAroundTable = 8; |
There was a problem hiding this comment.
@ChristianAlbrecht, we already have the tokens in JSON, right? Is it somehow exported to be accessed in JS?
org.knime.python3.scripting.nodes/js-src/src/components/PythonWorkspace.vue
Outdated
Show resolved
Hide resolved
| border-top: var(--kds-border-base-subtle); | ||
| } | ||
| </style> | ||
| @/python-initial-data-service |
There was a problem hiding this comment.
Not your change, but this line does not belong here. Some buggy VSCode tools created this line some time ago and it slipped through review. Can you please remove it.
There was a problem hiding this comment.
Do you mean the @/python-initial-data-service part?
org.knime.python3.scripting.nodes/js-src/src/components/PythonWorkspaceBody.vue
Outdated
Show resolved
Hide resolved
org.knime.python3.scripting.nodes/js-src/src/components/PythonWorkspaceBody.vue
Outdated
Show resolved
Hide resolved
org.knime.python3.scripting.nodes/js-src/src/components/PythonWorkspaceHeader.vue
Outdated
Show resolved
Hide resolved
org.knime.python3.scripting.nodes/js-src/src/components/PythonWorkspaceHeader.vue
Outdated
Show resolved
Hide resolved
senemdilli
force-pushed
the
KDS-547-python-scripting-temp-values
branch
from
08ccdb4 to
d347887
Compare
HedgehogCode
force-pushed
the
KDS-547-python-scripting-temp-values
branch
from
6baf009 to
d347887
Compare
HedgehogCode
force-pushed
the
KDS-547-python-scripting-temp-values
branch
from
d347887 to
3621f4b
Compare
| const rootResizeCallback = useDebounceFn((entries) => { | ||
| const rect = entries[0].contentRect; | ||
| totalWidth.value = rect.width; | ||
| totalWidth.value = rect.width - paddingAroundTable * 2; |
There was a problem hiding this comment.
The constant paddingAroundTable doesn't accurately describe what it represents. Consider renaming to horizontalPadding or sidePadding since it's multiplied by 2 to account for both left and right padding.
|
No description provided.