rusackas commented on code in PR #39257:
URL: https://github.com/apache/superset/pull/39257#discussion_r4140357936
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx:
##########
@@ -2240,6 +2295,8 @@ function DatasourceEditor({
expression: '',
})}
itemCellProps={{
+ metric_name: () => ({ className: 'datasource-key-cell' }),
+ verbose_name: () => ({ className: 'datasource-label-cell' }),
expression: () => ({
Review Comment:
Same as the other thread on the 240px maxWidth, that's gone now, Metrics
gets the wide layout too.
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx:
##########
@@ -741,6 +784,18 @@ function ColumnCollectionTable({
</StyledLabelWrapper>
),
type: d => (d ? <Label>{String(d)}</Label> : null),
+ expression: (v, onChange) => (
+ <TextAreaControl
+ initialValue={v as string}
+ onChange={onChange}
+ extraClasses={['datasource-sql-expression']}
+ language="sql"
+ offerEditInModal={false}
+ minLines={5}
+ textAreaStyles={{ minWidth: '100%', maxWidth: 'none' }}
+ resize="both"
+ />
+ ),
Review Comment:
This one's already addressed, the code uses `className` directly now instead
of `extraClasses`, and that flows through as part of `restProps` into the Ace
editor the same way `textAreaStyles` does, so the class applies.
##########
superset-frontend/src/components/Datasource/DatasourceModal/index.tsx:
##########
@@ -325,6 +332,8 @@ const DatasourceModal:
FunctionComponent<DatasourceModalProps> = ({
<StyledDatasourceModal
show={show}
onHide={onHide}
+ width={`${MODAL_WIDTH_VW}vw`}
+ maxWidth={`${MODAL_MAX_WIDTH}px`}
Review Comment:
There's some real tension here, dragging the resize handle won't shrink the
underlying vw width since that's relative to the viewport, not the wrapper.
That said it's not a regression from this PR, the modal was already fixed-width
under `responsive` (100vw) before this change. Feels more like a `Modal`
component limitation than something to fix in `DatasourceModal`, happy to open
a separate issue if it's actually causing problems for someone.
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx:
##########
@@ -505,32 +533,29 @@ function ColumnCollectionTable({
filterTerm,
filterFields,
}: ColumnCollectionTableProps): JSX.Element {
+ const tableColumns = isFeatureEnabled(FeatureFlag.EnableAdvancedDataTypes)
+ ? [
+ 'column_name',
+ ...(showExpression ? ['expression'] : []),
+ 'advanced_data_type',
+ 'type',
+ 'is_dttm',
+ 'filterable',
+ 'groupby',
+ ]
+ : [
+ 'column_name',
+ ...(showExpression ? ['expression'] : []),
+ 'type',
+ 'is_dttm',
+ 'filterable',
+ 'groupby',
+ ];
+
return (
<CollectionTable
- tableColumns={
- isFeatureEnabled(FeatureFlag.EnableAdvancedDataTypes)
- ? [
- 'column_name',
- 'advanced_data_type',
- 'type',
- 'is_dttm',
- 'filterable',
- 'groupby',
- ]
- : ['column_name', 'type', 'is_dttm', 'filterable', 'groupby']
- }
- sortColumns={
- isFeatureEnabled(FeatureFlag.EnableAdvancedDataTypes)
- ? [
- 'column_name',
- 'advanced_data_type',
- 'type',
- 'is_dttm',
- 'filterable',
- 'groupby',
- ]
- : ['column_name', 'type', 'is_dttm', 'filterable', 'groupby']
- }
+ tableColumns={tableColumns}
+ sortColumns={tableColumns}
Review Comment:
Good catch on the mechanism, `handleTableChange` does reset from
`propsCollection` rather than the local `collectionArray` when clearing a sort.
That's existing `CollectionTable` behavior though, this PR just newly turns
sorting on for these tables. I'd rather fix the reset-from-props path in
`CollectionTable` itself as a follow-up than roll it into this one.
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx:
##########
@@ -713,6 +744,18 @@ function ColumnCollectionTable({
),
type: d => (d ? <Label>{String(d)}</Label> : null),
advanced_data_type: d => <Label>{d as string}</Label>,
+ expression: (v, onChange) => (
+ <TextAreaControl
+ initialValue={v as string}
+ onChange={onChange}
Review Comment:
This is a real gap, if you hit "Sync columns from source" mid-edit the Ace
editor won't pick up the refreshed expression since `initialValue` only sets
Ace's `defaultValue` once. I don't want to key/remount it off the value though,
that'd blow away the cursor position on every keystroke. Worth its own fix
rather than rushing something here.
##########
superset-frontend/src/components/Datasource/DatasourceModal/index.tsx:
##########
@@ -376,7 +385,11 @@ const DatasourceModal:
FunctionComponent<DatasourceModalProps> = ({
responsive
resizable
resizableConfig={{
- defaultSize: { width: 'auto', height: `${MODAL_HEIGHT_VH}vh` },
+ defaultSize: {
+ width: `${MODAL_WIDTH_VW}vw`,
+ height: `${MODAL_HEIGHT_VH}vh`,
+ },
+ maxWidth: `${MODAL_MAX_WIDTH}px`,
maxHeight: `${MODAL_HEIGHT_VH}vh`,
}}
Review Comment:
This one's already handled, `Modal.tsx` merges `resizableConfig` through
`mergeResizableConfig()` instead of replacing it wholesale, so
`minWidth`/`minHeight` fall back to the shared defaults when a partial override
like this one doesn't set them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
