rusackas commented on code in PR #39257:
URL: https://github.com/apache/superset/pull/39257#discussion_r4140357018
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx:
##########
@@ -394,6 +394,27 @@ const StyledTableTabWrapper = styled.div`
vertical-align: middle;
}
+ &.wide-sql-layout {
+ .datasource-key-cell {
+ width: 30%;
+ }
+
+ .datasource-label-cell {
+ width: 20%;
+ }
+
+ .datasource-sql-cell {
+ width: 50%;
+ min-width: 480px;
+ }
Review Comment:
Fair point in isolation, but the modal body scrolls (`overflow: auto` on
`.ant-modal-body`), so worst case there's a horizontal scrollbar instead of a
broken layout. Keeping the SQL column readable felt more important than
shrinking below ~480px.
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx:
##########
@@ -394,6 +394,27 @@ const StyledTableTabWrapper = styled.div`
vertical-align: middle;
}
+ &.wide-sql-layout {
+ .datasource-key-cell {
+ width: 30%;
+ }
+
+ .datasource-label-cell {
+ width: 20%;
+ }
+
+ .datasource-sql-cell {
+ width: 50%;
+ min-width: 480px;
+ }
+
+ .datasource-sql-expression {
+ width: 100%;
+ min-width: 460px;
+ max-width: none;
+ }
Review Comment:
Same reasoning as the other thread on the SQL column width, the wrapper
scrolls rather than breaks, so I'd rather keep the floor than make the editor
unreadably narrow.
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx:
##########
@@ -2372,7 +2431,11 @@ function DatasourceEditor({
label: (
<CollectionTabTitle collection={sortedMetrics} title={t('Metrics')}
/>
),
- children: renderMetricCollection(),
+ children: (
+ <StyledTableTabWrapper className="wide-sql-layout">
Review Comment:
This one looks already fixed, the Metrics tab picks up `wide-sql-layout` now
and the `expression` cell uses `datasource-sql-cell` instead of the old inline
maxWidth. Should be resolved as of the latest push.
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx:
##########
@@ -435,9 +456,16 @@ const StyledButtonWrapper = styled.span`
`;
const checkboxGenerator = (
- d: boolean,
- onChange: (value: boolean) => void,
-): ReactNode => <CheckboxControl value={d} onChange={onChange} />;
+ d: unknown,
+ onChange: (value: unknown) => void,
+): ReactNode => (
Review Comment:
I think this is fine as is. `itemRenderers` is already typed as
`Record<string, (val: unknown, onChange: (value: unknown) => void, ...) =>
ReactNode>`, so once `checkboxGenerator` sits in the same map as the SQL text
renderers it has to widen to match that contract, TS won't let a
`boolean`-typed function satisfy an `unknown` one. `CheckboxControl` still gets
a coerced boolean via `Boolean(d)`, so nothing changes at runtime.
--
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]
