You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/08/05 20:21:22 UTC
[GitHub] [incubator-superset] nruhe opened a new pull request #10508: feat: Add antd to the codebase
nruhe opened a new pull request #10508:
URL: https://github.com/apache/incubator-superset/pull/10508
### SUMMARY
Introduces Ant Design as per [SIP-48](https://github.com/apache/incubator-superset/issues/10254), albeit with a few improvements.
1. Wrapping ant components in a config provider or ".ant" class will no longer be necessary. Just use Ant components directly!
2. Components are now being exposed through ./common/components folder and linting now errors if you try to include them via node_modules. This gives us the flexibility to later wrap any components in emotionJS as per [SIP-37](https://github.com/apache/incubator-superset/issues/9123).
As a follow up, we'll need to continue to tweak the Ant theme variables and styles as needed to match new design direction.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [X] Introduces new feature or API
- [ ] Removes existing feature or API
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] nruhe commented on a change in pull request #10508: feat: Add antd to the codebase
Posted by GitBox <gi...@apache.org>.
nruhe commented on a change in pull request #10508:
URL: https://github.com/apache/incubator-superset/pull/10508#discussion_r465380485
##########
File path: superset-frontend/stylesheets/antd/index.less
##########
@@ -0,0 +1,44 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+@import '~antd/lib/style/themes/index';
+@import '~antd/lib/style/mixins/index';
+@import '~antd/lib/style/core/base';
+
+*[class*='ant-'] {
+ @import '~antd/lib/style/core/global.less';
+}
+
+@import '~antd/lib/style/core/iconfont';
+@import '~antd/lib/style/core/motion';
+@import '~antd/lib/style/components.less';
+
+/*
+ Theme variables here: https://github.com/ant-design/ant-design/blob/master/components/style/themes/default.less
+*/
+@primary-color: #20a7c9;
Review comment:
I'm a little concerned with including that file directly since we might unintentionally overwrite variables we don't mean to. If we want to do this right, there should be a constants file that sources the common values used in the theme variables.
E.g.
```
// In constants.less
@superset-color-primary = #20a7c9;
// In variables.less
@primary-color = @superset-color-primary;
// In antd.less
@primary-color = @superset-color-primary;
```
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] ktmud commented on a change in pull request #10508: [WIP] feat: Add antd to the codebase
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10508:
URL: https://github.com/apache/incubator-superset/pull/10508#discussion_r464667708
##########
File path: superset-frontend/stylesheets/antd/index.less
##########
@@ -0,0 +1,45 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+@import '~antd/lib/style/themes/index';
+@import '~antd/lib/style/mixins/index';
+@import '~antd/lib/style/core/base';
+
+*[class*='ant-'] {
+ @import '~antd/lib/style/core/global.less';
+}
+
+@import '~antd/lib/style/core/iconfont';
+@import '~antd/lib/style/core/motion';
+@import "~antd/lib/style/components.less";
+
+/*
+ Theme variables here: https://github.com/ant-design/ant-design/blob/master/components/style/themes/default.less
+*/
+
+@primary-color: #20A7C9;
+@info-color: #66BCFE;
+@success-color: #59C189;
+@processing-color: #66BCFE;
+@error-color: #E04355;
+@highlight-color: #E04355;
+@warning-color: #FBC700;
+@normal-color: #d9d9d9;
+@white: #fff;
+@black: #000;
Review comment:
Missing new line.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] suddjian commented on a change in pull request #10508: Add antd to the codebase
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #10508:
URL: https://github.com/apache/incubator-superset/pull/10508#discussion_r464587304
##########
File path: superset-frontend/stylesheets/antd/index.less
##########
@@ -0,0 +1,26 @@
+@import '~antd/lib/style/themes/index';
+@import '~antd/lib/style/mixins/index';
+@import '~antd/lib/style/core/base';
+
+*[class*='ant-'] {
Review comment:
🔥
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] ktmud commented on a change in pull request #10508: feat: Add antd to the codebase
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10508:
URL: https://github.com/apache/incubator-superset/pull/10508#discussion_r465386486
##########
File path: superset-frontend/stylesheets/antd/index.less
##########
@@ -0,0 +1,44 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+@import '~antd/lib/style/themes/index';
+@import '~antd/lib/style/mixins/index';
+@import '~antd/lib/style/core/base';
+
+*[class*='ant-'] {
+ @import '~antd/lib/style/core/global.less';
+}
+
+@import '~antd/lib/style/core/iconfont';
+@import '~antd/lib/style/core/motion';
+@import '~antd/lib/style/components.less';
+
+/*
+ Theme variables here: https://github.com/ant-design/ant-design/blob/master/components/style/themes/default.less
+*/
+@primary-color: #20a7c9;
Review comment:
`variables.less` is kind of bloated right now and it's difficult to distinguish between our custom variables and the Bootstrap/Cosmos ones. I'm also concerned adding in ant variables may create a even bigger mess.
I think a larger question is do we want to keep `less` in our stack or do we want to slowly migrate away from it? The larger frontend community seems to have been [favoring](https://nextjs.org/docs/advanced-features/customizing-postcss-config) PostCSS, CSS variables, and CSS-in-JS recently. If we also believe these technologies are the future, we should probably limit our use of less to creating a base theme at build-time, and add runtime theme overrides using only styled components or css variables.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] nruhe commented on a change in pull request #10508: feat: Add antd to the codebase
Posted by GitBox <gi...@apache.org>.
nruhe commented on a change in pull request #10508:
URL: https://github.com/apache/incubator-superset/pull/10508#discussion_r465380485
##########
File path: superset-frontend/stylesheets/antd/index.less
##########
@@ -0,0 +1,44 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+@import '~antd/lib/style/themes/index';
+@import '~antd/lib/style/mixins/index';
+@import '~antd/lib/style/core/base';
+
+*[class*='ant-'] {
+ @import '~antd/lib/style/core/global.less';
+}
+
+@import '~antd/lib/style/core/iconfont';
+@import '~antd/lib/style/core/motion';
+@import '~antd/lib/style/components.less';
+
+/*
+ Theme variables here: https://github.com/ant-design/ant-design/blob/master/components/style/themes/default.less
+*/
+@primary-color: #20a7c9;
Review comment:
I'm a little concerned with including that file directly since we might unintentionally overwrite variables we don't mean to. If we want to do this right, there should be a constants file that sources the common variables.
E.g.
```
// In constants.less
@superset-color-primary = #20a7c9;
// In variables.less
@primary-color = @superset-color-primary;
// In antd.less
@primary-color = @superset-color-primary;
```
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] ktmud commented on a change in pull request #10508: feat: Add antd to the codebase
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10508:
URL: https://github.com/apache/incubator-superset/pull/10508#discussion_r465386486
##########
File path: superset-frontend/stylesheets/antd/index.less
##########
@@ -0,0 +1,44 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+@import '~antd/lib/style/themes/index';
+@import '~antd/lib/style/mixins/index';
+@import '~antd/lib/style/core/base';
+
+*[class*='ant-'] {
+ @import '~antd/lib/style/core/global.less';
+}
+
+@import '~antd/lib/style/core/iconfont';
+@import '~antd/lib/style/core/motion';
+@import '~antd/lib/style/components.less';
+
+/*
+ Theme variables here: https://github.com/ant-design/ant-design/blob/master/components/style/themes/default.less
+*/
+@primary-color: #20a7c9;
Review comment:
`variables.less` is kind of bloated right now and it's difficult to distinguish between our custom variables and the Bootstrap/Cosmos ones. Adding in ant variables will creating a even bigger mess.
I think a larger question is do we want to keep `less` in our stack or do we want to slowly migrate away from it? The larger frontend community seems to have been [favoring](https://nextjs.org/docs/advanced-features/customizing-postcss-config) PostCSS, CSS variables, and CSS-in-JS recently. If we also believe these technologies are the future, we should probably limit our use of less to creating a base theme at build-time, and add runtime theme overrides using only styled components or css variables.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] nytai commented on pull request #10508: feat: Add antd to the codebase
Posted by GitBox <gi...@apache.org>.
nytai commented on pull request #10508:
URL: https://github.com/apache/incubator-superset/pull/10508#issuecomment-669482257
closing/reopening to see if it triggers CI
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] mistercrunch commented on a change in pull request #10508: feat: Add antd to the codebase
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on a change in pull request #10508:
URL: https://github.com/apache/incubator-superset/pull/10508#discussion_r465369782
##########
File path: superset-frontend/stylesheets/antd/index.less
##########
@@ -0,0 +1,44 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+@import '~antd/lib/style/themes/index';
+@import '~antd/lib/style/mixins/index';
+@import '~antd/lib/style/core/base';
+
+*[class*='ant-'] {
+ @import '~antd/lib/style/core/global.less';
+}
+
+@import '~antd/lib/style/core/iconfont';
+@import '~antd/lib/style/core/motion';
+@import '~antd/lib/style/components.less';
+
+/*
+ Theme variables here: https://github.com/ant-design/ant-design/blob/master/components/style/themes/default.less
+*/
+@primary-color: #20a7c9;
Review comment:
referencing `variables.less` is a good first step
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] rusackas commented on a change in pull request #10508: feat: Add antd to the codebase
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10508:
URL: https://github.com/apache/incubator-superset/pull/10508#discussion_r465225687
##########
File path: superset-frontend/stylesheets/antd/index.less
##########
@@ -0,0 +1,44 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+@import '~antd/lib/style/themes/index';
+@import '~antd/lib/style/mixins/index';
+@import '~antd/lib/style/core/base';
+
+*[class*='ant-'] {
+ @import '~antd/lib/style/core/global.less';
+}
+
+@import '~antd/lib/style/core/iconfont';
+@import '~antd/lib/style/core/motion';
+@import '~antd/lib/style/components.less';
+
+/*
+ Theme variables here: https://github.com/ant-design/ant-design/blob/master/components/style/themes/default.less
+*/
+@primary-color: #20a7c9;
Review comment:
Or better yet, we could pull these variables in via JS using something like [this](https://github.com/mzohaibqc/react-app-rewire-antd-theme#readme) or (perhaps less ideal, but easier) [this](https://webpack.js.org/loaders/less-loader/#prependdata).
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] ktmud commented on a change in pull request #10508: feat: Add antd to the codebase
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10508:
URL: https://github.com/apache/incubator-superset/pull/10508#discussion_r465386486
##########
File path: superset-frontend/stylesheets/antd/index.less
##########
@@ -0,0 +1,44 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+@import '~antd/lib/style/themes/index';
+@import '~antd/lib/style/mixins/index';
+@import '~antd/lib/style/core/base';
+
+*[class*='ant-'] {
+ @import '~antd/lib/style/core/global.less';
+}
+
+@import '~antd/lib/style/core/iconfont';
+@import '~antd/lib/style/core/motion';
+@import '~antd/lib/style/components.less';
+
+/*
+ Theme variables here: https://github.com/ant-design/ant-design/blob/master/components/style/themes/default.less
+*/
+@primary-color: #20a7c9;
Review comment:
`variables.less` is kind of bloated right now and it's difficult to distinguish between our custom variables and the Bootstrap/Cosmos ones. I'm also worried that adding ant variables to this file may create a mess.
I think a larger question is do we want to keep `less` in our stack or do we want to slowly migrate away from it? The larger frontend community seems to have been [favoring](https://nextjs.org/docs/advanced-features/customizing-postcss-config) PostCSS, CSS variables, and CSS-in-JS recently. If we also believe these technologies are the future, we should probably limit our use of less to creating a base theme at build-time, and add runtime theme overrides using only styled components or css variables.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] nruhe commented on a change in pull request #10508: feat: Add antd to the codebase
Posted by GitBox <gi...@apache.org>.
nruhe commented on a change in pull request #10508:
URL: https://github.com/apache/incubator-superset/pull/10508#discussion_r465380485
##########
File path: superset-frontend/stylesheets/antd/index.less
##########
@@ -0,0 +1,44 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+@import '~antd/lib/style/themes/index';
+@import '~antd/lib/style/mixins/index';
+@import '~antd/lib/style/core/base';
+
+*[class*='ant-'] {
+ @import '~antd/lib/style/core/global.less';
+}
+
+@import '~antd/lib/style/core/iconfont';
+@import '~antd/lib/style/core/motion';
+@import '~antd/lib/style/components.less';
+
+/*
+ Theme variables here: https://github.com/ant-design/ant-design/blob/master/components/style/themes/default.less
+*/
+@primary-color: #20a7c9;
Review comment:
I'm a little concerned with including that file since we might unintentionally overwrite variables we don't mean to. If we want to do this right, there should be a root file that just namespaces the constants.
E.g.
```
// In constants.less
@superset-color-primary = #20a7c9;
// In variables.less
@primary-color = @superset-color-primary;
// In antd.less
@primary-color = @superset-color-primary;
```
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] rusackas commented on a change in pull request #10508: feat: Add antd to the codebase
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10508:
URL: https://github.com/apache/incubator-superset/pull/10508#discussion_r465217229
##########
File path: superset-frontend/.eslintrc.js
##########
@@ -87,6 +87,12 @@ module.exports = {
'no-prototype-builtins': 0,
'no-restricted-properties': 0,
'no-restricted-syntax': 0,
+ 'no-restricted-imports': ['error', {
+ 'paths': [{
+ 'name': 'antd',
Review comment:
👏
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] suddjian commented on a change in pull request #10508: Add antd to the codebase
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #10508:
URL: https://github.com/apache/incubator-superset/pull/10508#discussion_r464593672
##########
File path: superset-frontend/webpack.config.js
##########
@@ -293,6 +293,7 @@ const config = {
loader: 'less-loader',
options: {
sourceMap: isDevMode,
+ javascriptEnabled: true,
Review comment:
This option is usually nested [a bit deeper](https://ant.design/docs/react/use-with-create-react-app#Customize-Theme). Are you sure it works this way?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] suddjian commented on a change in pull request #10508: feat: Add antd to the codebase
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #10508:
URL: https://github.com/apache/incubator-superset/pull/10508#discussion_r465346751
##########
File path: superset-frontend/stylesheets/antd/index.less
##########
@@ -0,0 +1,44 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+@import '~antd/lib/style/themes/index';
+@import '~antd/lib/style/mixins/index';
+@import '~antd/lib/style/core/base';
+
+*[class*='ant-'] {
+ @import '~antd/lib/style/core/global.less';
+}
+
+@import '~antd/lib/style/core/iconfont';
+@import '~antd/lib/style/core/motion';
+@import '~antd/lib/style/components.less';
+
+/*
+ Theme variables here: https://github.com/ant-design/ant-design/blob/master/components/style/themes/default.less
+*/
+@primary-color: #20a7c9;
Review comment:
Yeah, finding a single-source solution to this will make a big difference for custom themes, or switching light and dark mode. I think ideally these variables would be set at runtime, not at build time.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] nytai merged pull request #10508: feat: Add antd to the codebase
Posted by GitBox <gi...@apache.org>.
nytai merged pull request #10508:
URL: https://github.com/apache/incubator-superset/pull/10508
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] ktmud commented on a change in pull request #10508: feat: Add antd to the codebase
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10508:
URL: https://github.com/apache/incubator-superset/pull/10508#discussion_r465386486
##########
File path: superset-frontend/stylesheets/antd/index.less
##########
@@ -0,0 +1,44 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+@import '~antd/lib/style/themes/index';
+@import '~antd/lib/style/mixins/index';
+@import '~antd/lib/style/core/base';
+
+*[class*='ant-'] {
+ @import '~antd/lib/style/core/global.less';
+}
+
+@import '~antd/lib/style/core/iconfont';
+@import '~antd/lib/style/core/motion';
+@import '~antd/lib/style/components.less';
+
+/*
+ Theme variables here: https://github.com/ant-design/ant-design/blob/master/components/style/themes/default.less
+*/
+@primary-color: #20a7c9;
Review comment:
`variables.less` is kind of bloated right now and it's difficult to distinguish between our custom variables and the Bootstrap/Cosmos ones. I'm also worried that adding ant variables to this file may create a even bigger mess.
I think a larger question is do we want to keep `less` in our stack or do we want to slowly migrate away from it? The larger frontend community seems to have been [favoring](https://nextjs.org/docs/advanced-features/customizing-postcss-config) PostCSS, CSS variables, and CSS-in-JS recently. If we also believe these technologies are the future, we should probably limit our use of less to creating a base theme at build-time, and add runtime theme overrides using only styled components or css variables.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] nruhe commented on a change in pull request #10508: [WIP] feat: Add antd to the codebase
Posted by GitBox <gi...@apache.org>.
nruhe commented on a change in pull request #10508:
URL: https://github.com/apache/incubator-superset/pull/10508#discussion_r464678662
##########
File path: superset-frontend/stylesheets/antd/index.less
##########
@@ -0,0 +1,45 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+@import '~antd/lib/style/themes/index';
+@import '~antd/lib/style/mixins/index';
+@import '~antd/lib/style/core/base';
+
+*[class*='ant-'] {
+ @import '~antd/lib/style/core/global.less';
+}
+
+@import '~antd/lib/style/core/iconfont';
+@import '~antd/lib/style/core/motion';
+@import "~antd/lib/style/components.less";
+
+/*
+ Theme variables here: https://github.com/ant-design/ant-design/blob/master/components/style/themes/default.less
+*/
+
+@primary-color: #20A7C9;
+@info-color: #66BCFE;
+@success-color: #59C189;
+@processing-color: #66BCFE;
+@error-color: #E04355;
+@highlight-color: #E04355;
+@warning-color: #FBC700;
+@normal-color: #d9d9d9;
+@white: #fff;
+@black: #000;
Review comment:
Added!
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] nytai closed pull request #10508: feat: Add antd to the codebase
Posted by GitBox <gi...@apache.org>.
nytai closed pull request #10508:
URL: https://github.com/apache/incubator-superset/pull/10508
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] nruhe commented on a change in pull request #10508: [WIP] feat: Add antd to the codebase
Posted by GitBox <gi...@apache.org>.
nruhe commented on a change in pull request #10508:
URL: https://github.com/apache/incubator-superset/pull/10508#discussion_r464594874
##########
File path: superset-frontend/webpack.config.js
##########
@@ -293,6 +293,7 @@ const config = {
loader: 'less-loader',
options: {
sourceMap: isDevMode,
+ javascriptEnabled: true,
Review comment:
Yeah, I believe the docs are referring to webpack 3. It breaks when it's wrong, so you know pretty fast.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] rusackas commented on a change in pull request #10508: feat: Add antd to the codebase
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10508:
URL: https://github.com/apache/incubator-superset/pull/10508#discussion_r465220080
##########
File path: superset-frontend/stylesheets/antd/index.less
##########
@@ -0,0 +1,44 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+@import '~antd/lib/style/themes/index';
+@import '~antd/lib/style/mixins/index';
+@import '~antd/lib/style/core/base';
+
+*[class*='ant-'] {
+ @import '~antd/lib/style/core/global.less';
+}
+
+@import '~antd/lib/style/core/iconfont';
+@import '~antd/lib/style/core/motion';
+@import '~antd/lib/style/components.less';
+
+/*
+ Theme variables here: https://github.com/ant-design/ant-design/blob/master/components/style/themes/default.less
+*/
+@primary-color: #20a7c9;
Review comment:
These colors should be pulled from `variables.less` rather than being hard-coded here.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org