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 2021/04/27 18:07:28 UTC

[GitHub] [superset] geido opened a new pull request #14318: feat: Dynamic imports for the Icons component

geido opened a new pull request #14318:
URL: https://github.com/apache/superset/pull/14318


   ### SUMMARY
   It enhances the `Icons` component with dynamic imports and auto-generation of `aria-label`. As a POC, it changes the existing `alert-solid` icon with the new `Icons.AlertSolid` implementation.
   
   ##### Remarks:
   As the Icons are now dynamically imported, test cases need to wait for the Icons to be imported and re-rendered to pass without warnings. In a react-testing-library setup, this translates to using the `findBy*` promise-based approach as shown in this PR.
   
   ### BEFORE / AFTER:
   
   #### Alert
   ![alert_b4](https://user-images.githubusercontent.com/60598000/115922201-47f86980-a485-11eb-9d81-248909206542.png)
   ![alert_after](https://user-images.githubusercontent.com/60598000/115922207-49c22d00-a485-11eb-857a-f00bb118c61a.png)
   
   #### Datasource Control
   ![datasource_control_b4](https://user-images.githubusercontent.com/60598000/115922241-534b9500-a485-11eb-9ee9-c7d066183ebe.png)
   ![datasource_control_after](https://user-images.githubusercontent.com/60598000/115922255-56468580-a485-11eb-95a0-bdeb7f328198.png)
   
   #### Datasource Editor
   ![datasource_editor_alert_b4](https://user-images.githubusercontent.com/60598000/115922292-6199b100-a485-11eb-8935-0b357d403bf5.png)
   ![datasource_editor_alert_after](https://user-images.githubusercontent.com/60598000/115922294-63637480-a485-11eb-99f3-2c57547ce320.png)
   
   #### Alerts & Reports
   ![alerts_reports_b4](https://user-images.githubusercontent.com/60598000/116118761-5386a300-a6c6-11eb-8c84-5ae113481452.png)
   ![alerts_reports_after](https://user-images.githubusercontent.com/60598000/116118773-55e8fd00-a6c6-11eb-86f2-8295d38b6cb1.png)
   
   
   


-- 
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] [superset] codecov[bot] edited a comment on pull request #14318: feat: Dynamic imports for the Icons component

Posted by GitBox <gi...@apache.org>.

codecov[bot] edited a comment on pull request #14318:
URL: https://github.com/apache/superset/pull/14318#issuecomment-827069364


   # [Codecov](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#14318](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d7a4f64) into [master](https://codecov.io/gh/apache/superset/commit/87a895cc4acaf0fe5aa91f4d4db6c86781f6961e?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (87a895c) will **increase** coverage by `0.06%`.
   > The diff coverage is `34.94%`.
   
   > :exclamation: Current head d7a4f64 differs from pull request most recent head ec0c9da. Consider uploading reports for the commit ec0c9da to get more accurate results
   [![Impacted file tree graph](https://codecov.io/gh/apache/superset/pull/14318/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #14318      +/-   ##
   ==========================================
   + Coverage   76.95%   77.02%   +0.06%     
   ==========================================
     Files         954      954              
     Lines       48207    48031     -176     
     Branches     6008     5985      -23     
   ==========================================
   - Hits        37097    36994     -103     
   + Misses      10915    10839      -76     
   - Partials      195      198       +3     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | javascript | `71.91% <34.94%> (+0.27%)` | :arrow_up: |
   | presto | `?` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...re/components/controls/DatasourceControl/index.jsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9EYXRhc291cmNlQ29udHJvbC9pbmRleC5qc3g=) | `86.84% <ø> (ø)` | |
   | [superset-frontend/src/components/Icons/index.tsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvSWNvbnMvaW5kZXgudHN4) | `17.69% <17.69%> (ø)` | |
   | [...rc/views/CRUD/alert/components/AlertStatusIcon.tsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvYWxlcnQvY29tcG9uZW50cy9BbGVydFN0YXR1c0ljb24udHN4) | `47.76% <53.33%> (+0.06%)` | :arrow_up: |
   | [superset-frontend/src/components/Alert/index.tsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQWxlcnQvaW5kZXgudHN4) | `100.00% <100.00%> (ø)` | |
   | [superset-frontend/src/components/Icons/Icon.tsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvSWNvbnMvSWNvbi50c3g=) | `100.00% <100.00%> (ø)` | |
   | [...nd/src/components/WarningIconWithTooltip/index.tsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvV2FybmluZ0ljb25XaXRoVG9vbHRpcC9pbmRleC50c3g=) | `100.00% <100.00%> (+10.00%)` | :arrow_up: |
   | [...nd/src/dashboard/components/FiltersBadge/index.tsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0ZpbHRlcnNCYWRnZS9pbmRleC50c3g=) | `100.00% <100.00%> (ø)` | |
   | [...ters/FiltersConfigModal/FiltersConfigForm/state.ts](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyc0NvbmZpZ01vZGFsL0ZpbHRlcnNDb25maWdGb3JtL3N0YXRlLnRz) | `74.00% <0.00%> (-26.00%)` | :arrow_down: |
   | [superset/utils/hashing.py](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvaGFzaGluZy5weQ==) | `75.00% <0.00%> (-25.00%)` | :arrow_down: |
   | [...nts/controls/DateFilterControl/DateFilterLabel.tsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9EYXRlRmlsdGVyQ29udHJvbC9EYXRlRmlsdGVyTGFiZWwudHN4) | `73.72% <0.00%> (-9.33%)` | :arrow_down: |
   | ... and [41 more](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [87a895c...ec0c9da](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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] [superset] geido closed pull request #14318: feat: Dynamic imports for the Icons component

Posted by GitBox <gi...@apache.org>.

geido closed pull request #14318:
URL: https://github.com/apache/superset/pull/14318


   


-- 
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] [superset] geido commented on a change in pull request #14318: feat: Dynamic imports for the Icons component

Posted by GitBox <gi...@apache.org>.

geido commented on a change in pull request #14318:
URL: https://github.com/apache/superset/pull/14318#discussion_r621346427



##########
File path: superset-frontend/src/components/Icons/index.tsx
##########
@@ -0,0 +1,175 @@
+/**
+ * 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 React from 'react';
+import AntdEnhancedIcons from './AntdEnhanced';
+import { Icon } from './Icon';
+import IconType from './IconType';
+
+export default {
+  ...AntdEnhancedIcons,
+  Alert: (props: IconType) => <Icon path="alert" {...props} />,

Review comment:
       That's a clever idea!!




-- 
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] [superset] michael-s-molina commented on a change in pull request #14318: feat: Dynamic imports for the Icons component

Posted by GitBox <gi...@apache.org>.

michael-s-molina commented on a change in pull request #14318:
URL: https://github.com/apache/superset/pull/14318#discussion_r622347680



##########
File path: superset-frontend/src/components/Icons/index.tsx
##########
@@ -0,0 +1,166 @@
+/**
+ * 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 React from 'react';
+import _ from 'lodash';
+import AntdEnhancedIcons from './AntdEnhanced';
+import Icon from './Icon';
+import IconType from './IconType';
+
+const iconFilePaths = [

Review comment:
       Maybe change to `iconNames`? The path is being computed inside `Icon`.




-- 
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] [superset] geido edited a comment on pull request #14318: feat: Dynamic imports for the Icons component

Posted by GitBox <gi...@apache.org>.

geido edited a comment on pull request #14318:
URL: https://github.com/apache/superset/pull/14318#issuecomment-829330817


   > Is there a flash of blank-icons and then icons slowly appearing on screen if there are a whole bunch of them being lazy-loaded?
   
   As you can see in this video, in an unrealistic scenario where all 100+ icons are being loaded at once, you will see for a moment them loading. I haven't noticed this behavior in a real-life scenario though. Loading is basically instant. 
   [Play Video](https://www.awesomescreenshot.com/video/3567643?key=e867c391f279ea7e676b11a9da10c6a4)
   
   > Is there indeed a smaller bundle when using this dynamic import technique vs the "import ever svg into the component" plan? I would assume so, but it'd be nice to verify. You might be able to do npm run build -- --analyzeBundle=true to get an idea of this.
   
   Haven't noticed any particular improvement in the bundle size
   
   CC @rusackas 


-- 
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] [superset] codecov[bot] edited a comment on pull request #14318: feat: Dynamic imports for the Icons component

Posted by GitBox <gi...@apache.org>.

codecov[bot] edited a comment on pull request #14318:
URL: https://github.com/apache/superset/pull/14318#issuecomment-827069364


   # [Codecov](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#14318](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d46c610) into [master](https://codecov.io/gh/apache/superset/commit/1c1626165124ec3a17acf79b1558d86d206d7466?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1c16261) will **increase** coverage by `0.02%`.
   > The diff coverage is `77.14%`.
   
   > :exclamation: Current head d46c610 differs from pull request most recent head c857a26. Consider uploading reports for the commit c857a26 to get more accurate results
   [![Impacted file tree graph](https://codecov.io/gh/apache/superset/pull/14318/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #14318      +/-   ##
   ==========================================
   + Coverage   76.81%   76.84%   +0.02%     
   ==========================================
     Files         955      954       -1     
     Lines       48249    48002     -247     
     Branches     6032     6021      -11     
   ==========================================
   - Hits        37063    36885     -178     
   + Misses      10990    10920      -70     
   - Partials      196      197       +1     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | javascript | `71.53% <77.14%> (+0.16%)` | :arrow_up: |
   | presto | `?` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...re/components/controls/DatasourceControl/index.jsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9EYXRhc291cmNlQ29udHJvbC9pbmRleC5qc3g=) | `86.84% <ø> (ø)` | |
   | [...set-frontend/src/components/Icons/AntdEnhanced.tsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvSWNvbnMvQW50ZEVuaGFuY2VkLnRzeA==) | `87.50% <50.00%> (ø)` | |
   | [...rc/views/CRUD/alert/components/AlertStatusIcon.tsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvYWxlcnQvY29tcG9uZW50cy9BbGVydFN0YXR1c0ljb24udHN4) | `47.76% <53.33%> (+0.06%)` | :arrow_up: |
   | [superset-frontend/src/components/Icons/Icon.tsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvSWNvbnMvSWNvbi50c3g=) | `96.00% <95.00%> (-4.00%)` | :arrow_down: |
   | [superset-frontend/src/components/Alert/index.tsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQWxlcnQvaW5kZXgudHN4) | `100.00% <100.00%> (ø)` | |
   | [superset-frontend/src/components/Icons/index.tsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvSWNvbnMvaW5kZXgudHN4) | `100.00% <100.00%> (ø)` | |
   | [...nd/src/components/WarningIconWithTooltip/index.tsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvV2FybmluZ0ljb25XaXRoVG9vbHRpcC9pbmRleC50c3g=) | `100.00% <100.00%> (+10.00%)` | :arrow_up: |
   | [...nd/src/dashboard/components/FiltersBadge/index.tsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0ZpbHRlcnNCYWRnZS9pbmRleC50c3g=) | `100.00% <100.00%> (ø)` | |
   | [...tersConfigModal/FiltersConfigForm/DefaultValue.tsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyc0NvbmZpZ01vZGFsL0ZpbHRlcnNDb25maWdGb3JtL0RlZmF1bHRWYWx1ZS50c3g=) | `27.77% <0.00%> (-61.12%)` | :arrow_down: |
   | [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `84.42% <0.00%> (-5.48%)` | :arrow_down: |
   | ... and [20 more](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [1c16261...c857a26](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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] [superset] rusackas commented on a change in pull request #14318: feat: Dynamic imports for the Icons component

Posted by GitBox <gi...@apache.org>.

rusackas commented on a change in pull request #14318:
URL: https://github.com/apache/superset/pull/14318#discussion_r620889639



##########
File path: superset-frontend/src/components/Icons/index.tsx
##########
@@ -0,0 +1,175 @@
+/**
+ * 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 React from 'react';
+import AntdEnhancedIcons from './AntdEnhanced';
+import { Icon } from './Icon';
+import IconType from './IconType';
+
+export default {
+  ...AntdEnhancedIcons,
+  Alert: (props: IconType) => <Icon path="alert" {...props} />,

Review comment:
       Not sure if this makes sense, but maybe worth exploring, to shrink the file some... if they all fit the same pattern of naming conventions and such.
   
   ```js
   // import lodash at the top...
   
   const iconPaths = ['alert_solid', 'alert_solid_small']; // etc... this is the list we would need to maintain.
   const iconOverrides = {};
   iconPaths.forEach(path => {
    const keyName = _.startCase('path').replace(/ /g,''); // lodash strips dashes and capitalizes, then we remove spaces.
    iconOverrides[keyName] = (props: IconType) => <Icon path={path} {...props} />;
   });
   
   export default {
     ...AntdEnhancedIcons,
     ...iconOverrides,
   }
   
   ```




-- 
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] [superset] rusackas commented on pull request #14318: feat: Dynamic imports for the Icons component

Posted by GitBox <gi...@apache.org>.

rusackas commented on pull request #14318:
URL: https://github.com/apache/superset/pull/14318#issuecomment-828968739


   I'm SO tempted to merge this... I think the code LGTM. One minor naming nit remains as an open comment. The bigger issue is perf testing:
   1) Is there a flash of blank-icons and then icons slowly appearing on screen if there are a whole bunch of them being lazy-loaded?
   2) Is there indeed a smaller bundle when using this dynamic import technique vs the "import ever svg into the component" plan? I would assume so, but it'd be nice to verify. You might be able to do `npm run build -- --analyzeBundle=true` to get an idea of this.


-- 
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] [superset] codecov[bot] edited a comment on pull request #14318: feat: Dynamic imports for the Icons component

Posted by GitBox <gi...@apache.org>.

codecov[bot] edited a comment on pull request #14318:
URL: https://github.com/apache/superset/pull/14318#issuecomment-827069364


   # [Codecov](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#14318](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d7a4f64) into [master](https://codecov.io/gh/apache/superset/commit/41ccebce91ac6b763824201cfd131794a7a620d6?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (41ccebc) will **increase** coverage by `0.03%`.
   > The diff coverage is `34.94%`.
   
   > :exclamation: Current head d7a4f64 differs from pull request most recent head b8d4548. Consider uploading reports for the commit b8d4548 to get more accurate results
   [![Impacted file tree graph](https://codecov.io/gh/apache/superset/pull/14318/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #14318      +/-   ##
   ==========================================
   + Coverage   76.98%   77.02%   +0.03%     
   ==========================================
     Files         954      954              
     Lines       48197    48031     -166     
     Branches     6006     5985      -21     
   ==========================================
   - Hits        37103    36994     -109     
   + Misses      10898    10839      -59     
   - Partials      196      198       +2     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | javascript | `71.91% <34.94%> (+0.21%)` | :arrow_up: |
   | presto | `?` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...re/components/controls/DatasourceControl/index.jsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9EYXRhc291cmNlQ29udHJvbC9pbmRleC5qc3g=) | `86.84% <ø> (ø)` | |
   | [superset-frontend/src/components/Icons/index.tsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvSWNvbnMvaW5kZXgudHN4) | `17.69% <17.69%> (ø)` | |
   | [...rc/views/CRUD/alert/components/AlertStatusIcon.tsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvYWxlcnQvY29tcG9uZW50cy9BbGVydFN0YXR1c0ljb24udHN4) | `47.76% <53.33%> (+0.06%)` | :arrow_up: |
   | [superset-frontend/src/components/Alert/index.tsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQWxlcnQvaW5kZXgudHN4) | `100.00% <100.00%> (ø)` | |
   | [superset-frontend/src/components/Icons/Icon.tsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvSWNvbnMvSWNvbi50c3g=) | `100.00% <100.00%> (ø)` | |
   | [...nd/src/components/WarningIconWithTooltip/index.tsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvV2FybmluZ0ljb25XaXRoVG9vbHRpcC9pbmRleC50c3g=) | `100.00% <100.00%> (+10.00%)` | :arrow_up: |
   | [...nd/src/dashboard/components/FiltersBadge/index.tsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0ZpbHRlcnNCYWRnZS9pbmRleC50c3g=) | `100.00% <100.00%> (ø)` | |
   | [superset/utils/hashing.py](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvaGFzaGluZy5weQ==) | `75.00% <0.00%> (-25.00%)` | :arrow_down: |
   | [...nts/controls/DateFilterControl/DateFilterLabel.tsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9EYXRlRmlsdGVyQ29udHJvbC9EYXRlRmlsdGVyTGFiZWwudHN4) | `73.72% <0.00%> (-9.33%)` | :arrow_down: |
   | [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `84.42% <0.00%> (-5.90%)` | :arrow_down: |
   | ... and [41 more](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [87a895c...b8d4548](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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] [superset] codecov[bot] edited a comment on pull request #14318: feat: Dynamic imports for the Icons component

Posted by GitBox <gi...@apache.org>.

codecov[bot] edited a comment on pull request #14318:
URL: https://github.com/apache/superset/pull/14318#issuecomment-827069364


   # [Codecov](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#14318](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d7a4f64) into [master](https://codecov.io/gh/apache/superset/commit/6875a1a9e264c04f15736f42848825a25ad2c019?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6875a1a) will **decrease** coverage by `0.04%`.
   > The diff coverage is `34.94%`.
   
   > :exclamation: Current head d7a4f64 differs from pull request most recent head 89089e2. Consider uploading reports for the commit 89089e2 to get more accurate results
   [![Impacted file tree graph](https://codecov.io/gh/apache/superset/pull/14318/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #14318      +/-   ##
   ==========================================
   - Coverage   77.06%   77.02%   -0.05%     
   ==========================================
     Files         954      954              
     Lines       48141    48031     -110     
     Branches     5987     5985       -2     
   ==========================================
   - Hits        37098    36994     -104     
   + Misses      10846    10839       -7     
   - Partials      197      198       +1     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | javascript | `71.91% <34.94%> (-0.12%)` | :arrow_down: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...re/components/controls/DatasourceControl/index.jsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9EYXRhc291cmNlQ29udHJvbC9pbmRleC5qc3g=) | `86.84% <ø> (ø)` | |
   | [superset-frontend/src/components/Icons/index.tsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvSWNvbnMvaW5kZXgudHN4) | `17.69% <17.69%> (ø)` | |
   | [...rc/views/CRUD/alert/components/AlertStatusIcon.tsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvYWxlcnQvY29tcG9uZW50cy9BbGVydFN0YXR1c0ljb24udHN4) | `47.76% <53.33%> (+0.06%)` | :arrow_up: |
   | [superset-frontend/src/components/Alert/index.tsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQWxlcnQvaW5kZXgudHN4) | `100.00% <100.00%> (ø)` | |
   | [superset-frontend/src/components/Icons/Icon.tsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvSWNvbnMvSWNvbi50c3g=) | `100.00% <100.00%> (ø)` | |
   | [...nd/src/components/WarningIconWithTooltip/index.tsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvV2FybmluZ0ljb25XaXRoVG9vbHRpcC9pbmRleC50c3g=) | `100.00% <100.00%> (+10.00%)` | :arrow_up: |
   | [...nd/src/dashboard/components/FiltersBadge/index.tsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0ZpbHRlcnNCYWRnZS9pbmRleC50c3g=) | `100.00% <100.00%> (ø)` | |
   | [...tend/src/filters/components/Select/controlPanel.ts](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9TZWxlY3QvY29udHJvbFBhbmVsLnRz) | `63.63% <0.00%> (ø)` | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [6875a1a...89089e2](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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] [superset] geido commented on a change in pull request #14318: feat: Dynamic imports for the Icons component

Posted by GitBox <gi...@apache.org>.

geido commented on a change in pull request #14318:
URL: https://github.com/apache/superset/pull/14318#discussion_r621329021



##########
File path: superset-frontend/src/components/Icons/Icon.tsx
##########
@@ -17,34 +17,52 @@
  * under the License.
  */
 
-import React from 'react';
+import React, { useEffect, useRef, useState } from 'react';
 import AntdIcon from '@ant-design/icons';
 import { styled } from '@superset-ui/core';
-import { CustomIconComponentProps } from '@ant-design/icons/lib/components/Icon';
+import { ReactComponent as TransparentIcon } from 'images/icons/transparent.svg';
 import IconType from './IconType';
 
 // eslint-disable-next-line @typescript-eslint/no-unused-vars
-const EnhancedIcon = ({ iconColor, iconSize, ...rest }: IconType) => (
+const AntdIconComponent = ({ iconColor, iconSize, ...rest }: IconType) => (
   <AntdIcon viewBox={rest.viewBox || '0 0 24 24'} {...rest} />
 );
 
-const Icon = styled(EnhancedIcon)<IconType>`
+const StyledIcon = styled(AntdIconComponent)<IconType>`
   ${({ iconColor }) => iconColor && `color: ${iconColor};`};
   font-size: ${({ iconSize, theme }) =>
     iconSize ? `${theme.typography.sizes[iconSize]}px` : '24px'};
 `;
 
-export const renderIcon = (
-  SVGComponent:
-    | React.ComponentClass<
-        CustomIconComponentProps | React.SVGProps<SVGSVGElement>,
-        any
-      >
-    | React.FunctionComponent<
-        CustomIconComponentProps | React.SVGProps<SVGSVGElement>
-      >
-    | undefined,
-  props: IconType,
-) => <Icon component={SVGComponent} {...props} />;
-
-export default Icon;
+interface IconProps extends IconType {
+  path: string;
+}
+
+export const Icon = (props: IconProps) => {
+  const { path, ...iconProps } = props;
+  const [, setLoaded] = useState(false);
+  const ImportedSVG = useRef<React.FC<React.SVGProps<SVGSVGElement>>>();
+  const name = path.replace('_', '-');
+
+  useEffect(() => {
+    async function importIcon(): Promise<void> {
+      ImportedSVG.current = (
+        await import(
+          `!!@svgr/webpack?-svgo,+titleProp,+ref!images/icons/${path}.svg`
+        )
+      ).default;
+      setLoaded(true);
+    }
+    importIcon();
+  }, [path, ImportedSVG]);
+
+  return (
+    <StyledIcon
+      component={ImportedSVG.current || TransparentIcon}

Review comment:
       Hey Evan. Thanks for your input. Imho this approach is better because the TransparentIcon can get the same properties of the actual icon in terms of size, aria-label, etc in order to keep the status of the UI consistent until the final icon is loaded. What I forgot to do here is to pass the right properties to the Transparent icon so that my goal is achieved. I will try that in my next update.




-- 
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] [superset] geido commented on a change in pull request #14318: feat: Dynamic imports for the Icons component

Posted by GitBox <gi...@apache.org>.

geido commented on a change in pull request #14318:
URL: https://github.com/apache/superset/pull/14318#discussion_r621329021



##########
File path: superset-frontend/src/components/Icons/Icon.tsx
##########
@@ -17,34 +17,52 @@
  * under the License.
  */
 
-import React from 'react';
+import React, { useEffect, useRef, useState } from 'react';
 import AntdIcon from '@ant-design/icons';
 import { styled } from '@superset-ui/core';
-import { CustomIconComponentProps } from '@ant-design/icons/lib/components/Icon';
+import { ReactComponent as TransparentIcon } from 'images/icons/transparent.svg';
 import IconType from './IconType';
 
 // eslint-disable-next-line @typescript-eslint/no-unused-vars
-const EnhancedIcon = ({ iconColor, iconSize, ...rest }: IconType) => (
+const AntdIconComponent = ({ iconColor, iconSize, ...rest }: IconType) => (
   <AntdIcon viewBox={rest.viewBox || '0 0 24 24'} {...rest} />
 );
 
-const Icon = styled(EnhancedIcon)<IconType>`
+const StyledIcon = styled(AntdIconComponent)<IconType>`
   ${({ iconColor }) => iconColor && `color: ${iconColor};`};
   font-size: ${({ iconSize, theme }) =>
     iconSize ? `${theme.typography.sizes[iconSize]}px` : '24px'};
 `;
 
-export const renderIcon = (
-  SVGComponent:
-    | React.ComponentClass<
-        CustomIconComponentProps | React.SVGProps<SVGSVGElement>,
-        any
-      >
-    | React.FunctionComponent<
-        CustomIconComponentProps | React.SVGProps<SVGSVGElement>
-      >
-    | undefined,
-  props: IconType,
-) => <Icon component={SVGComponent} {...props} />;
-
-export default Icon;
+interface IconProps extends IconType {
+  path: string;
+}
+
+export const Icon = (props: IconProps) => {
+  const { path, ...iconProps } = props;
+  const [, setLoaded] = useState(false);
+  const ImportedSVG = useRef<React.FC<React.SVGProps<SVGSVGElement>>>();
+  const name = path.replace('_', '-');
+
+  useEffect(() => {
+    async function importIcon(): Promise<void> {
+      ImportedSVG.current = (
+        await import(
+          `!!@svgr/webpack?-svgo,+titleProp,+ref!images/icons/${path}.svg`
+        )
+      ).default;
+      setLoaded(true);
+    }
+    importIcon();
+  }, [path, ImportedSVG]);
+
+  return (
+    <StyledIcon
+      component={ImportedSVG.current || TransparentIcon}

Review comment:
       Hey Evan. Thanks for your input. Imho this approach is better because the TransparentIcon can get the same properties of the actual icon in terms of size, aria-label, etc in order to keep the status of the UI consistent until the final icon is loaded.




-- 
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] [superset] rusackas commented on a change in pull request #14318: feat: Dynamic imports for the Icons component

Posted by GitBox <gi...@apache.org>.

rusackas commented on a change in pull request #14318:
URL: https://github.com/apache/superset/pull/14318#discussion_r620895681



##########
File path: superset-frontend/src/components/Icons/Icon.tsx
##########
@@ -17,34 +17,52 @@
  * under the License.
  */
 
-import React from 'react';
+import React, { useEffect, useRef, useState } from 'react';
 import AntdIcon from '@ant-design/icons';
 import { styled } from '@superset-ui/core';
-import { CustomIconComponentProps } from '@ant-design/icons/lib/components/Icon';
+import { ReactComponent as TransparentIcon } from 'images/icons/transparent.svg';
 import IconType from './IconType';
 
 // eslint-disable-next-line @typescript-eslint/no-unused-vars
-const EnhancedIcon = ({ iconColor, iconSize, ...rest }: IconType) => (
+const AntdIconComponent = ({ iconColor, iconSize, ...rest }: IconType) => (
   <AntdIcon viewBox={rest.viewBox || '0 0 24 24'} {...rest} />
 );
 
-const Icon = styled(EnhancedIcon)<IconType>`
+const StyledIcon = styled(AntdIconComponent)<IconType>`
   ${({ iconColor }) => iconColor && `color: ${iconColor};`};
   font-size: ${({ iconSize, theme }) =>
     iconSize ? `${theme.typography.sizes[iconSize]}px` : '24px'};
 `;
 
-export const renderIcon = (
-  SVGComponent:
-    | React.ComponentClass<
-        CustomIconComponentProps | React.SVGProps<SVGSVGElement>,
-        any
-      >
-    | React.FunctionComponent<
-        CustomIconComponentProps | React.SVGProps<SVGSVGElement>
-      >
-    | undefined,
-  props: IconType,
-) => <Icon component={SVGComponent} {...props} />;
-
-export default Icon;
+interface IconProps extends IconType {
+  path: string;

Review comment:
       nit, but it might be worth calling this filePath since that's what it's really referencing, since `path` also sounds like an SVG path




-- 
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] [superset] rusackas commented on a change in pull request #14318: feat: Dynamic imports for the Icons component

Posted by GitBox <gi...@apache.org>.

rusackas commented on a change in pull request #14318:
URL: https://github.com/apache/superset/pull/14318#discussion_r622733729



##########
File path: superset-frontend/src/components/Icons/index.tsx
##########
@@ -0,0 +1,166 @@
+/**
+ * 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 React from 'react';
+import _ from 'lodash';
+import AntdEnhancedIcons from './AntdEnhanced';
+import Icon from './Icon';
+import IconType from './IconType';
+
+const iconFilePaths = [

Review comment:
       True... though iconName could be confusing as well. IconFileName might be the most explicit :P




-- 
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] [superset] rusackas commented on a change in pull request #14318: feat: Dynamic imports for the Icons component

Posted by GitBox <gi...@apache.org>.

rusackas commented on a change in pull request #14318:
URL: https://github.com/apache/superset/pull/14318#discussion_r620879086



##########
File path: superset-frontend/src/components/Icons/Icon.tsx
##########
@@ -17,34 +17,52 @@
  * under the License.
  */
 
-import React from 'react';
+import React, { useEffect, useRef, useState } from 'react';
 import AntdIcon from '@ant-design/icons';
 import { styled } from '@superset-ui/core';
-import { CustomIconComponentProps } from '@ant-design/icons/lib/components/Icon';
+import { ReactComponent as TransparentIcon } from 'images/icons/transparent.svg';
 import IconType from './IconType';
 
 // eslint-disable-next-line @typescript-eslint/no-unused-vars
-const EnhancedIcon = ({ iconColor, iconSize, ...rest }: IconType) => (
+const AntdIconComponent = ({ iconColor, iconSize, ...rest }: IconType) => (
   <AntdIcon viewBox={rest.viewBox || '0 0 24 24'} {...rest} />
 );
 
-const Icon = styled(EnhancedIcon)<IconType>`
+const StyledIcon = styled(AntdIconComponent)<IconType>`
   ${({ iconColor }) => iconColor && `color: ${iconColor};`};
   font-size: ${({ iconSize, theme }) =>
     iconSize ? `${theme.typography.sizes[iconSize]}px` : '24px'};

Review comment:
       Not part of this PR, but:
   
   This value is typed for `tsx` files, but you could perhaps pass a bad `iconSize` into this via `jsx` files. If that's a risk worth considering, you could mitigate it with `${theme.typography.sizes[iconSize] || theme.typography.sizes.m}px`
   ```suggestion
       iconSize ? `${theme.typography.sizes[iconSize]}px` : '24px'};
   ```




-- 
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] [superset] geido commented on a change in pull request #14318: feat: Dynamic imports for the Icons component

Posted by GitBox <gi...@apache.org>.

geido commented on a change in pull request #14318:
URL: https://github.com/apache/superset/pull/14318#discussion_r621497585



##########
File path: superset-frontend/src/components/Icons/Icon.tsx
##########
@@ -17,34 +17,52 @@
  * under the License.
  */
 
-import React from 'react';
+import React, { useEffect, useRef, useState } from 'react';
 import AntdIcon from '@ant-design/icons';
 import { styled } from '@superset-ui/core';
-import { CustomIconComponentProps } from '@ant-design/icons/lib/components/Icon';
+import { ReactComponent as TransparentIcon } from 'images/icons/transparent.svg';
 import IconType from './IconType';
 
 // eslint-disable-next-line @typescript-eslint/no-unused-vars
-const EnhancedIcon = ({ iconColor, iconSize, ...rest }: IconType) => (
+const AntdIconComponent = ({ iconColor, iconSize, ...rest }: IconType) => (
   <AntdIcon viewBox={rest.viewBox || '0 0 24 24'} {...rest} />
 );
 
-const Icon = styled(EnhancedIcon)<IconType>`
+const StyledIcon = styled(AntdIconComponent)<IconType>`
   ${({ iconColor }) => iconColor && `color: ${iconColor};`};
   font-size: ${({ iconSize, theme }) =>
     iconSize ? `${theme.typography.sizes[iconSize]}px` : '24px'};
 `;
 
-export const renderIcon = (
-  SVGComponent:
-    | React.ComponentClass<
-        CustomIconComponentProps | React.SVGProps<SVGSVGElement>,
-        any
-      >
-    | React.FunctionComponent<
-        CustomIconComponentProps | React.SVGProps<SVGSVGElement>
-      >
-    | undefined,
-  props: IconType,
-) => <Icon component={SVGComponent} {...props} />;
-
-export default Icon;
+interface IconProps extends IconType {
+  path: string;
+}
+
+export const Icon = (props: IconProps) => {
+  const { path, ...iconProps } = props;
+  const [, setLoaded] = useState(false);
+  const ImportedSVG = useRef<React.FC<React.SVGProps<SVGSVGElement>>>();
+  const name = path.replace('_', '-');
+
+  useEffect(() => {
+    async function importIcon(): Promise<void> {
+      ImportedSVG.current = (
+        await import(
+          `!!@svgr/webpack?-svgo,+titleProp,+ref!images/icons/${path}.svg`
+        )
+      ).default;
+      setLoaded(true);
+    }
+    importIcon();
+  }, [path, ImportedSVG]);
+
+  return (
+    <StyledIcon
+      component={ImportedSVG.current || TransparentIcon}

Review comment:
       Probably also with your approach that would work, but Antd will fire warnings for the type being invalid




-- 
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] [superset] geido commented on a change in pull request #14318: feat: Dynamic imports for the Icons component

Posted by GitBox <gi...@apache.org>.

geido commented on a change in pull request #14318:
URL: https://github.com/apache/superset/pull/14318#discussion_r621497585



##########
File path: superset-frontend/src/components/Icons/Icon.tsx
##########
@@ -17,34 +17,52 @@
  * under the License.
  */
 
-import React from 'react';
+import React, { useEffect, useRef, useState } from 'react';
 import AntdIcon from '@ant-design/icons';
 import { styled } from '@superset-ui/core';
-import { CustomIconComponentProps } from '@ant-design/icons/lib/components/Icon';
+import { ReactComponent as TransparentIcon } from 'images/icons/transparent.svg';
 import IconType from './IconType';
 
 // eslint-disable-next-line @typescript-eslint/no-unused-vars
-const EnhancedIcon = ({ iconColor, iconSize, ...rest }: IconType) => (
+const AntdIconComponent = ({ iconColor, iconSize, ...rest }: IconType) => (
   <AntdIcon viewBox={rest.viewBox || '0 0 24 24'} {...rest} />
 );
 
-const Icon = styled(EnhancedIcon)<IconType>`
+const StyledIcon = styled(AntdIconComponent)<IconType>`
   ${({ iconColor }) => iconColor && `color: ${iconColor};`};
   font-size: ${({ iconSize, theme }) =>
     iconSize ? `${theme.typography.sizes[iconSize]}px` : '24px'};
 `;
 
-export const renderIcon = (
-  SVGComponent:
-    | React.ComponentClass<
-        CustomIconComponentProps | React.SVGProps<SVGSVGElement>,
-        any
-      >
-    | React.FunctionComponent<
-        CustomIconComponentProps | React.SVGProps<SVGSVGElement>
-      >
-    | undefined,
-  props: IconType,
-) => <Icon component={SVGComponent} {...props} />;
-
-export default Icon;
+interface IconProps extends IconType {
+  path: string;
+}
+
+export const Icon = (props: IconProps) => {
+  const { path, ...iconProps } = props;
+  const [, setLoaded] = useState(false);
+  const ImportedSVG = useRef<React.FC<React.SVGProps<SVGSVGElement>>>();
+  const name = path.replace('_', '-');
+
+  useEffect(() => {
+    async function importIcon(): Promise<void> {
+      ImportedSVG.current = (
+        await import(
+          `!!@svgr/webpack?-svgo,+titleProp,+ref!images/icons/${path}.svg`
+        )
+      ).default;
+      setLoaded(true);
+    }
+    importIcon();
+  }, [path, ImportedSVG]);
+
+  return (
+    <StyledIcon
+      component={ImportedSVG.current || TransparentIcon}

Review comment:
       Probably also with your approach that would work, but Antd would probably fire warnings for the type being invalid




-- 
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] [superset] rusackas commented on pull request #14318: feat: Dynamic imports for the Icons component

Posted by GitBox <gi...@apache.org>.

rusackas commented on pull request #14318:
URL: https://github.com/apache/superset/pull/14318#issuecomment-828939932


   /testenv up


-- 
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] [superset] codecov[bot] edited a comment on pull request #14318: feat: Dynamic imports for the Icons component

Posted by GitBox <gi...@apache.org>.

codecov[bot] edited a comment on pull request #14318:
URL: https://github.com/apache/superset/pull/14318#issuecomment-827069364


   # [Codecov](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#14318](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d46c610) into [master](https://codecov.io/gh/apache/superset/commit/87a895cc4acaf0fe5aa91f4d4db6c86781f6961e?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (87a895c) will **decrease** coverage by `0.11%`.
   > The diff coverage is `66.66%`.
   
   > :exclamation: Current head d46c610 differs from pull request most recent head ec0c9da. Consider uploading reports for the commit ec0c9da to get more accurate results
   [![Impacted file tree graph](https://codecov.io/gh/apache/superset/pull/14318/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #14318      +/-   ##
   ==========================================
   - Coverage   76.95%   76.84%   -0.12%     
   ==========================================
     Files         954      954              
     Lines       48207    48002     -205     
     Branches     6008     6021      +13     
   ==========================================
   - Hits        37097    36885     -212     
   - Misses      10915    10920       +5     
   - Partials      195      197       +2     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | javascript | `71.53% <66.66%> (-0.11%)` | :arrow_down: |
   | presto | `?` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...tiveFilters/FilterBar/FilterSets/FiltersHeader.tsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyQmFyL0ZpbHRlclNldHMvRmlsdGVyc0hlYWRlci50c3g=) | `91.66% <0.00%> (-5.56%)` | :arrow_down: |
   | [...mponents/nativeFilters/FiltersConfigModal/utils.ts](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyc0NvbmZpZ01vZGFsL3V0aWxzLnRz) | `65.78% <ø> (ø)` | |
   | [...re/components/controls/DatasourceControl/index.jsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9EYXRhc291cmNlQ29udHJvbC9pbmRleC5qc3g=) | `86.84% <ø> (ø)` | |
   | [...ontend/src/filters/components/Select/buildQuery.ts](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9TZWxlY3QvYnVpbGRRdWVyeS50cw==) | `25.00% <0.00%> (-8.34%)` | :arrow_down: |
   | [...et-frontend/src/filters/components/Select/types.ts](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9TZWxlY3QvdHlwZXMudHM=) | `100.00% <ø> (ø)` | |
   | [...onfigModal/FiltersConfigForm/FiltersConfigForm.tsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyc0NvbmZpZ01vZGFsL0ZpbHRlcnNDb25maWdGb3JtL0ZpbHRlcnNDb25maWdGb3JtLnRzeA==) | `68.92% <38.46%> (+2.46%)` | :arrow_up: |
   | [...set-frontend/src/components/Icons/AntdEnhanced.tsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvSWNvbnMvQW50ZEVuaGFuY2VkLnRzeA==) | `87.50% <50.00%> (ø)` | |
   | [...rc/views/CRUD/alert/components/AlertStatusIcon.tsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvYWxlcnQvY29tcG9uZW50cy9BbGVydFN0YXR1c0ljb24udHN4) | `47.76% <53.33%> (+0.06%)` | :arrow_up: |
   | [...nd/src/dashboard/components/nativeFilters/utils.ts](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvdXRpbHMudHM=) | `79.48% <66.66%> (-1.60%)` | :arrow_down: |
   | [superset-frontend/src/reduxUtils.ts](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3JlZHV4VXRpbHMudHM=) | `69.04% <71.42%> (-0.58%)` | :arrow_down: |
   | ... and [20 more](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [87a895c...ec0c9da](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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] [superset] codecov[bot] edited a comment on pull request #14318: feat: Dynamic imports for the Icons component

Posted by GitBox <gi...@apache.org>.

codecov[bot] edited a comment on pull request #14318:
URL: https://github.com/apache/superset/pull/14318#issuecomment-827069364


   # [Codecov](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#14318](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c857a26) into [master](https://codecov.io/gh/apache/superset/commit/1c1626165124ec3a17acf79b1558d86d206d7466?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1c16261) will **increase** coverage by `0.12%`.
   > The diff coverage is `77.14%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/superset/pull/14318/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #14318      +/-   ##
   ==========================================
   + Coverage   76.81%   76.93%   +0.12%     
   ==========================================
     Files         955      955              
     Lines       48249    48021     -228     
     Branches     6032     6032              
   ==========================================
   - Hits        37063    36947     -116     
   + Misses      10990    10876     -114     
   - Partials      196      198       +2     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | javascript | `71.57% <77.14%> (+0.20%)` | :arrow_up: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...re/components/controls/DatasourceControl/index.jsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9EYXRhc291cmNlQ29udHJvbC9pbmRleC5qc3g=) | `86.84% <ø> (ø)` | |
   | [...set-frontend/src/components/Icons/AntdEnhanced.tsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvSWNvbnMvQW50ZEVuaGFuY2VkLnRzeA==) | `87.50% <50.00%> (ø)` | |
   | [...rc/views/CRUD/alert/components/AlertStatusIcon.tsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvYWxlcnQvY29tcG9uZW50cy9BbGVydFN0YXR1c0ljb24udHN4) | `47.76% <53.33%> (+0.06%)` | :arrow_up: |
   | [superset-frontend/src/components/Icons/Icon.tsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvSWNvbnMvSWNvbi50c3g=) | `96.00% <95.00%> (-4.00%)` | :arrow_down: |
   | [superset-frontend/src/components/Alert/index.tsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQWxlcnQvaW5kZXgudHN4) | `100.00% <100.00%> (ø)` | |
   | [superset-frontend/src/components/Icons/index.tsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvSWNvbnMvaW5kZXgudHN4) | `100.00% <100.00%> (ø)` | |
   | [...nd/src/components/WarningIconWithTooltip/index.tsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvV2FybmluZ0ljb25XaXRoVG9vbHRpcC9pbmRleC50c3g=) | `100.00% <100.00%> (+10.00%)` | :arrow_up: |
   | [...nd/src/dashboard/components/FiltersBadge/index.tsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0ZpbHRlcnNCYWRnZS9pbmRleC50c3g=) | `100.00% <100.00%> (ø)` | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [1c16261...c857a26](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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] [superset] geido commented on pull request #14318: feat: Dynamic imports for the Icons component

Posted by GitBox <gi...@apache.org>.

geido commented on pull request #14318:
URL: https://github.com/apache/superset/pull/14318#issuecomment-829330817


   > Is there a flash of blank-icons and then icons slowly appearing on screen if there are a whole bunch of them being lazy-loaded?
   
   As you can see in this video, in an unrealistic scenario where all 100+ icons are being loaded at once, you will see for a moment them loading. I haven't noticed this behaviour in a real-life scenario though. Loading is basically instant. https://www.awesomescreenshot.com/video/3567643?key=e867c391f279ea7e676b11a9da10c6a4
   
   
   > Is there indeed a smaller bundle when using this dynamic import technique vs the "import ever svg into the component" plan? I would assume so, but it'd be nice to verify. You might be able to do npm run build -- --analyzeBundle=true to get an idea of this.
   
   Haven't noticed any particular improvement in the bundle size
   


-- 
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] [superset] codecov[bot] commented on pull request #14318: feat: Dynamic imports for the Icons component

Posted by GitBox <gi...@apache.org>.

codecov[bot] commented on pull request #14318:
URL: https://github.com/apache/superset/pull/14318#issuecomment-827069364


   # [Codecov](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#14318](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d7a4f64) into [master](https://codecov.io/gh/apache/superset/commit/6875a1a9e264c04f15736f42848825a25ad2c019?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6875a1a) will **decrease** coverage by `0.04%`.
   > The diff coverage is `34.94%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/superset/pull/14318/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #14318      +/-   ##
   ==========================================
   - Coverage   77.06%   77.02%   -0.05%     
   ==========================================
     Files         954      954              
     Lines       48141    48031     -110     
     Branches     5987     5985       -2     
   ==========================================
   - Hits        37098    36994     -104     
   + Misses      10846    10839       -7     
   - Partials      197      198       +1     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | javascript | `71.91% <34.94%> (-0.12%)` | :arrow_down: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...re/components/controls/DatasourceControl/index.jsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9EYXRhc291cmNlQ29udHJvbC9pbmRleC5qc3g=) | `86.84% <ø> (ø)` | |
   | [superset-frontend/src/components/Icons/index.tsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvSWNvbnMvaW5kZXgudHN4) | `17.69% <17.69%> (ø)` | |
   | [...rc/views/CRUD/alert/components/AlertStatusIcon.tsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvYWxlcnQvY29tcG9uZW50cy9BbGVydFN0YXR1c0ljb24udHN4) | `47.76% <53.33%> (+0.06%)` | :arrow_up: |
   | [superset-frontend/src/components/Alert/index.tsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQWxlcnQvaW5kZXgudHN4) | `100.00% <100.00%> (ø)` | |
   | [superset-frontend/src/components/Icons/Icon.tsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvSWNvbnMvSWNvbi50c3g=) | `100.00% <100.00%> (ø)` | |
   | [...nd/src/components/WarningIconWithTooltip/index.tsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvV2FybmluZ0ljb25XaXRoVG9vbHRpcC9pbmRleC50c3g=) | `100.00% <100.00%> (+10.00%)` | :arrow_up: |
   | [...nd/src/dashboard/components/FiltersBadge/index.tsx](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0ZpbHRlcnNCYWRnZS9pbmRleC50c3g=) | `100.00% <100.00%> (ø)` | |
   | [...tend/src/filters/components/Select/controlPanel.ts](https://codecov.io/gh/apache/superset/pull/14318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9TZWxlY3QvY29udHJvbFBhbmVsLnRz) | `63.63% <0.00%> (ø)` | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [6875a1a...d7a4f64](https://codecov.io/gh/apache/superset/pull/14318?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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] [superset] rusackas commented on a change in pull request #14318: feat: Dynamic imports for the Icons component

Posted by GitBox <gi...@apache.org>.

rusackas commented on a change in pull request #14318:
URL: https://github.com/apache/superset/pull/14318#discussion_r620877067



##########
File path: superset-frontend/src/components/Icons/Icon.tsx
##########
@@ -17,34 +17,52 @@
  * under the License.
  */
 
-import React from 'react';
+import React, { useEffect, useRef, useState } from 'react';
 import AntdIcon from '@ant-design/icons';
 import { styled } from '@superset-ui/core';
-import { CustomIconComponentProps } from '@ant-design/icons/lib/components/Icon';
+import { ReactComponent as TransparentIcon } from 'images/icons/transparent.svg';
 import IconType from './IconType';
 
 // eslint-disable-next-line @typescript-eslint/no-unused-vars
-const EnhancedIcon = ({ iconColor, iconSize, ...rest }: IconType) => (
+const AntdIconComponent = ({ iconColor, iconSize, ...rest }: IconType) => (
   <AntdIcon viewBox={rest.viewBox || '0 0 24 24'} {...rest} />
 );
 
-const Icon = styled(EnhancedIcon)<IconType>`
+const StyledIcon = styled(AntdIconComponent)<IconType>`
   ${({ iconColor }) => iconColor && `color: ${iconColor};`};
   font-size: ${({ iconSize, theme }) =>
     iconSize ? `${theme.typography.sizes[iconSize]}px` : '24px'};
 `;
 
-export const renderIcon = (
-  SVGComponent:
-    | React.ComponentClass<
-        CustomIconComponentProps | React.SVGProps<SVGSVGElement>,
-        any
-      >
-    | React.FunctionComponent<
-        CustomIconComponentProps | React.SVGProps<SVGSVGElement>
-      >
-    | undefined,
-  props: IconType,
-) => <Icon component={SVGComponent} {...props} />;
-
-export default Icon;
+interface IconProps extends IconType {
+  path: string;
+}
+
+export const Icon = (props: IconProps) => {
+  const { path, ...iconProps } = props;
+  const [, setLoaded] = useState(false);
+  const ImportedSVG = useRef<React.FC<React.SVGProps<SVGSVGElement>>>();
+  const name = path.replace('_', '-');
+
+  useEffect(() => {
+    async function importIcon(): Promise<void> {
+      ImportedSVG.current = (
+        await import(
+          `!!@svgr/webpack?-svgo,+titleProp,+ref!images/icons/${path}.svg`
+        )
+      ).default;
+      setLoaded(true);
+    }
+    importIcon();
+  }, [path, ImportedSVG]);
+
+  return (
+    <StyledIcon
+      component={ImportedSVG.current || TransparentIcon}

Review comment:
       It doesn't seem like it _should_ work, but it _appears_ we might be able to get away with `() => false` instead of `TransparentIcon` if it's not too hacky. I don't mind the transparentIcon idea though, really.




-- 
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] [superset] github-actions[bot] commented on pull request #14318: feat: Dynamic imports for the Icons component

Posted by GitBox <gi...@apache.org>.

github-actions[bot] commented on pull request #14318:
URL: https://github.com/apache/superset/pull/14318#issuecomment-827307971


   @pkdotson Ephemeral environment spinning up at http://52.89.241.110:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.


-- 
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] [superset] pkdotson commented on pull request #14318: feat: Dynamic imports for the Icons component

Posted by GitBox <gi...@apache.org>.

pkdotson commented on pull request #14318:
URL: https://github.com/apache/superset/pull/14318#issuecomment-827307438


   /testenv up


-- 
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] [superset] rusackas merged pull request #14318: feat: Dynamic imports for the Icons component

Posted by GitBox <gi...@apache.org>.

rusackas merged pull request #14318:
URL: https://github.com/apache/superset/pull/14318


   


-- 
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] [superset] github-actions[bot] commented on pull request #14318: feat: Dynamic imports for the Icons component

Posted by GitBox <gi...@apache.org>.

github-actions[bot] commented on pull request #14318:
URL: https://github.com/apache/superset/pull/14318#issuecomment-829526509


   Ephemeral environment shutdown and build artifacts deleted.


-- 
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] [superset] github-actions[bot] commented on pull request #14318: feat: Dynamic imports for the Icons component

Posted by GitBox <gi...@apache.org>.

github-actions[bot] commented on pull request #14318:
URL: https://github.com/apache/superset/pull/14318#issuecomment-827806981


   Ephemeral environment shutdown and build artifacts deleted.


-- 
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] [superset] rusackas commented on a change in pull request #14318: chore: Icon migration - Iterarion 1

Posted by GitBox <gi...@apache.org>.

rusackas commented on a change in pull request #14318:
URL: https://github.com/apache/superset/pull/14318#discussion_r619551383



##########
File path: superset-frontend/src/views/CRUD/alert/components/AlertStatusIcon.tsx
##########
@@ -16,34 +16,31 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import { styled, t } from '@superset-ui/core';
+import { t, supersetTheme } from '@superset-ui/core';

Review comment:
       let's try not to use the supersetTheme directly, but get our variables via the ThemeProvider. You can use `withTheme` (HOC) or `useTheme` (hook) in most cases.




-- 
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] [superset] geido edited a comment on pull request #14318: feat: Dynamic imports for the Icons component

Posted by GitBox <gi...@apache.org>.

geido edited a comment on pull request #14318:
URL: https://github.com/apache/superset/pull/14318#issuecomment-829330817


   > Is there a flash of blank-icons and then icons slowly appearing on screen if there are a whole bunch of them being lazy-loaded?
   
   As you can see in this video, in an unrealistic scenario where all 100+ icons are being loaded at once, you will see for a moment them loading. I haven't noticed this behaviour in a real-life scenario though. Loading is basically instant. https://www.awesomescreenshot.com/video/3567643?key=e867c391f279ea7e676b11a9da10c6a4
   
   
   > Is there indeed a smaller bundle when using this dynamic import technique vs the "import ever svg into the component" plan? I would assume so, but it'd be nice to verify. You might be able to do npm run build -- --analyzeBundle=true to get an idea of this.
   
   Haven't noticed any particular improvement in the bundle size
   
   CC @rusackas 


-- 
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] [superset] github-actions[bot] commented on pull request #14318: feat: Dynamic imports for the Icons component

Posted by GitBox <gi...@apache.org>.

github-actions[bot] commented on pull request #14318:
URL: https://github.com/apache/superset/pull/14318#issuecomment-828940487


   @rusackas Ephemeral environment spinning up at http://54.203.92.75:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.


-- 
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] [superset] geido edited a comment on pull request #14318: feat: Dynamic imports for the Icons component

Posted by GitBox <gi...@apache.org>.

geido edited a comment on pull request #14318:
URL: https://github.com/apache/superset/pull/14318#issuecomment-829330817


   > Is there a flash of blank-icons and then icons slowly appearing on screen if there are a whole bunch of them being lazy-loaded?
   
   As you can see in this video, in an unrealistic scenario where all 100+ icons are being loaded at once, you will see for a moment them loading. I haven't noticed this behaviour in a real-life scenario though. Loading is basically instant. 
   [Play Video](https://www.awesomescreenshot.com/video/3567643?key=e867c391f279ea7e676b11a9da10c6a4)
   
   > Is there indeed a smaller bundle when using this dynamic import technique vs the "import ever svg into the component" plan? I would assume so, but it'd be nice to verify. You might be able to do npm run build -- --analyzeBundle=true to get an idea of this.
   
   Haven't noticed any particular improvement in the bundle size
   
   CC @rusackas 


-- 
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] [superset] rusackas commented on a change in pull request #14318: feat: Dynamic imports for the Icons component

Posted by GitBox <gi...@apache.org>.

rusackas commented on a change in pull request #14318:
URL: https://github.com/apache/superset/pull/14318#discussion_r620889639



##########
File path: superset-frontend/src/components/Icons/index.tsx
##########
@@ -0,0 +1,175 @@
+/**
+ * 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 React from 'react';
+import AntdEnhancedIcons from './AntdEnhanced';
+import { Icon } from './Icon';
+import IconType from './IconType';
+
+export default {
+  ...AntdEnhancedIcons,
+  Alert: (props: IconType) => <Icon path="alert" {...props} />,

Review comment:
       Not sure if this makes sense, but maybe worth exploring, to shrink the file some... if they all fit the same pattern of naming conventions and such.
   
   ```js
   // import lodash at the top...
   
   const iconPaths = [alert_solid, alert_solid_small]; // etc... this is the list we would need to maintain.
   const iconOverrides = {};
   iconPaths.forEach(path => {
    const keyName = _.startCase('path').replace(/ /g,''); // lodash strips dashes and capitalizes, then we remove spaces.
    iconOverrides[keyName] = (props: IconType) => <Icon path={path} {...props} />;
   });
   
   export default {
     ...AntdEnhancedIcons,
     ...iconOverrides,
   }
   
   ```




-- 
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