rusackas commented on a change in pull request #12200:
URL:
https://github.com/apache/incubator-superset/pull/12200#discussion_r551635457
##########
File path: superset-frontend/src/datasource/DatasourceEditor.jsx
##########
@@ -261,7 +262,11 @@ StackedField.propTypes = {
};
function FormContainer({ children }) {
- return <Well style={{ marginTop: 20 }}>{children}</Well>;
+ return (
+ <Card style={{ marginTop: 20 }} bodyStyle={{ padding: 15 }}>
Review comment:
It looks like we might not really _need_ that 20px top margin.
As for the bodyStyle's padding, I would love to use the Theme variables here
too. There are a few options:
`1` use `withTheme` to use the `gridUnit`s here directly (4 gridUnits for
16px would be close enough)
`2a` remove the bodyStyle prop and use a **class name** (e.g. 'padded') and
use that className in the component's Emotion stylesheet to add the padding
(again, with `gridUnit`s)
`2b` remove the bodyStyle prop and use a **prop** (e.g. 'padded') and use
that prop in the component's Emotion stylesheet to apply the padding (again,
with `gridUnit`s)
`3` use emotion's `css` prop, e.g.
```
<Card
css={theme => ({
marginTop: theme.gridUnit * 5,
padding: theme.gridUnit * 4,
})}
>
```
I'd prefer either 2a or 2b, personally.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
