feat: Dynamic imports for the Icons component #14318

geido · 2021-04-23T19:22:21Z

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

Datasource Control

Datasource Editor

Alerts & Reports

superset-frontend/src/views/CRUD/alert/components/AlertStatusIcon.tsx

codecov · 2021-04-26T18:54:26Z

Codecov Report

Merging #14318 (c857a26) into master (1c16261) will increase coverage by 0.12%.
The diff coverage is 77.14%.

@@            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%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...re/components/controls/DatasourceControl/index.jsx	`86.84% <ø> (ø)`
...set-frontend/src/components/Icons/AntdEnhanced.tsx	`87.50% <50.00%> (ø)`
...rc/views/CRUD/alert/components/AlertStatusIcon.tsx	`47.76% <53.33%> (+0.06%)`	⬆️
superset-frontend/src/components/Icons/Icon.tsx	`96.00% <95.00%> (-4.00%)`	⬇️
superset-frontend/src/components/Alert/index.tsx	`100.00% <100.00%> (ø)`
superset-frontend/src/components/Icons/index.tsx	`100.00% <100.00%> (ø)`
...nd/src/components/WarningIconWithTooltip/index.tsx	`100.00% <100.00%> (+10.00%)`	⬆️
...nd/src/dashboard/components/FiltersBadge/index.tsx	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1c16261...c857a26. Read the comment docs.

pkdotson · 2021-04-27T04:40:31Z

/testenv up

github-actions · 2021-04-27T04:42:34Z

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

superset-frontend/src/components/Icons/Icon.tsx

superset-frontend/src/components/Icons/index.tsx

superset-frontend/src/components/Icons/Icon.tsx

…tion_iterarion_1

github-actions · 2021-04-27T18:07:42Z

Ephemeral environment shutdown and build artifacts deleted.

pkdotson

LGTM! Thanks for implementing this!

superset-frontend/src/components/Icons/index.tsx

rusackas · 2021-04-29T05:00:18Z

/testenv up

github-actions · 2021-04-29T05:02:05Z

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

rusackas · 2021-04-29T06:18:12Z

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:

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

…tion_iterarion_1

geido · 2021-04-29T15:21:43Z

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

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

rusackas

LGTM. Let's do this!!! :D

github-actions · 2021-04-29T19:26:54Z

Ephemeral environment shutdown and build artifacts deleted.

* master: (38 commits) refactor(native-filters): allow cascading only for filter_select (apache#14441) test(maximize-chart): Add tests to maximize chart action (apache#14371) fix: fixing mysql error message (apache#14416) feat: Logic added to limiting factor column in Query model (apache#13521) change relationship (apache#14435) fix: bootstrap data permissions (apache#14348) fix: parse simple string error message values (apache#14360) chore: add stack trace to all calls of logger.error (apache#14382) update README with new docs and recordings (apache#14432) Renamed impyla from implya in impala.mdx and Renamed PIP package impyla from impala in index.mdx (apache#14425) fix(native-filters): fix filter scope error (apache#14426) feat: Adding limiting_factor column to Query model (apache#14234) feat: Add etag caching to dashboard APIs (apache#14357) chore: Moves Card to the components folder (apache#14139) feat: Dynamic imports for the Icons component (apache#14318) feat: Support env vars configuration for WebSocket server (apache#14398) fix: SQLLab role permissions (apache#14372) fix(native-filters): always show filters without dataset (apache#14409) fix error getting partitionQuery from table.partition (apache#14369) refactor: Boostrap to AntD - Tabs (apache#14048) ...

* Add aria-label and twotone * Enhance LazyIcon * Fix tests and solve ject warnings * Add new line * Revert package-lock to master * Fix failing test * Implement icon overrides * Fix failing storybook * Clean up * Improve var name

geido added 2 commits April 23, 2021 22:19

Add aria-label and twotone

6de57c6

Merge

fdc4d3a

superset-github-bot bot added the Flexiana label Apr 23, 2021

rusackas reviewed Apr 24, 2021

View reviewed changes

superset-frontend/src/views/CRUD/alert/components/AlertStatusIcon.tsx Outdated Show resolved Hide resolved

geido added 2 commits April 25, 2021 20:52

Enhance LazyIcon

bc68a92

Fix tests and solve ject warnings

ff8ab16

geido changed the title ~~chore: Icon migration - Iterarion 1~~ feat: Dynamic imports for the Icons component Apr 26, 2021

Merge

a227d00

geido marked this pull request as ready for review April 26, 2021 17:10

geido added 2 commits April 26, 2021 20:24

Add new line

7b8850b

Revert package-lock to master

2633b77

pull-request-size bot added the size/XL label Apr 26, 2021

Fix failing test

d7a4f64

rusackas requested review from nytai, pkdotson and suddjian April 27, 2021 05:14

rusackas reviewed Apr 27, 2021

View reviewed changes

superset-frontend/src/components/Icons/Icon.tsx Show resolved Hide resolved

rusackas reviewed Apr 27, 2021

View reviewed changes

superset-frontend/src/components/Icons/Icon.tsx Outdated Show resolved Hide resolved

rusackas reviewed Apr 27, 2021

View reviewed changes

superset-frontend/src/components/Icons/index.tsx Outdated Show resolved Hide resolved

rusackas reviewed Apr 27, 2021

View reviewed changes

superset-frontend/src/components/Icons/Icon.tsx Outdated Show resolved Hide resolved

geido added 4 commits April 27, 2021 19:28

Implement icon overrides

89089e2

Merge remote-tracking branch 'upstream/master' into chore/icons_migra…

b8d4548

…tion_iterarion_1

Fix failing storybook

912c7a2

Clean up

ec0c9da

geido closed this Apr 27, 2021

geido reopened this Apr 27, 2021

pkdotson approved these changes Apr 27, 2021

View reviewed changes

michael-s-molina reviewed Apr 28, 2021

View reviewed changes

superset-frontend/src/components/Icons/index.tsx Outdated Show resolved Hide resolved

geido added 2 commits April 29, 2021 17:34

Improve var name

2dad5c2

Merge remote-tracking branch 'upstream/master' into chore/icons_migra…

c857a26

…tion_iterarion_1

rusackas approved these changes Apr 29, 2021

View reviewed changes

rusackas merged commit 545e257 into apache:master Apr 29, 2021

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 First shipped in 1.2.0 labels Mar 12, 2024

feat: Dynamic imports for the Icons component #14318

feat: Dynamic imports for the Icons component #14318

Uh oh!

Conversation

geido commented Apr 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

SUMMARY

Remarks:

BEFORE / AFTER:

Alert

Datasource Control

Datasource Editor

Alerts & Reports

Uh oh!

Uh oh!

codecov bot commented Apr 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pkdotson commented Apr 27, 2021

Uh oh!

github-actions bot commented Apr 27, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Apr 27, 2021

Uh oh!

pkdotson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rusackas commented Apr 29, 2021

Uh oh!

github-actions bot commented Apr 29, 2021

Uh oh!

rusackas commented Apr 29, 2021

Uh oh!

geido commented Apr 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rusackas left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

geido commented Apr 23, 2021 •

edited

Loading

codecov bot commented Apr 26, 2021 •

edited

Loading

geido commented Apr 29, 2021 •

edited

Loading