Skip to content

refactor: New Icon system with Enhanced Antd Custom Icon#12229

Merged
rusackas merged 28 commits into
apache:masterfrom
geido:custom_icons_to_antd
Feb 25, 2021
Merged

refactor: New Icon system with Enhanced Antd Custom Icon#12229
rusackas merged 28 commits into
apache:masterfrom
geido:custom_icons_to_antd

Conversation

@geido
Copy link
Copy Markdown
Member

@geido geido commented Dec 30, 2020

SUMMARY

This PR introduces an entirely new Icon system that uses a combination of the Antd Custom Icon and Emotion while consolidating the usage of icons.

  • Use existing Antd icons with the ability to enhance them
  • Use custom icons trough the Antd Custom Icon component and the ability to enhance them
  • Adds the ability to override any Antd existing icon with a custom one

As an example implementation, the following sections have been changed to use some of the icons under the new icon system:

  • Charts lists
  • Dashboards lists
  • Saved Queries lists
  • Datasets lists
  • Databases lists
  • Explore - Edit Datasource columns

BEFORE

charts-list-BEFORE

dashboard-list-trash-BEFORE

dashboard-lists-cardview-BEFORE

databases-lists-BEFORE

datasets-list-BEFORE

saved-query-list-trash-BEFORE

Screen Shot 2021-02-03 at 17 14 41

AFTER

charts-list-AFTER

dashboard-list-trash-AFTER

dashboard-lists-cardview-AFTER

databases-lists-AFTER

datasets-list-AFTER

saved-query-list-trash-AFTER

Screen Shot 2021-02-03 at 20 56 09

TEST PLAN

  1. Go to any list
  2. Hover your mouse on a list item
  3. Make sure the trash icon appears and works as intended

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

@geido
Copy link
Copy Markdown
Member Author

geido commented Dec 30, 2020

@rusackas @junlincc

@rusackas rusackas added the hold! On hold label Jan 6, 2021
@junlincc junlincc removed the hold! On hold label Jan 22, 2021
@junlincc junlincc requested a review from rusackas January 22, 2021 17:05
@pull-request-size pull-request-size Bot added size/L and removed size/S labels Jan 25, 2021
@geido geido changed the title refactor: Custom icons to antd refactor: Trash icon enhanced to Antd Icon in lists Jan 25, 2021
@geido geido marked this pull request as ready for review January 25, 2021 19:40
@codecov-io
Copy link
Copy Markdown

codecov-io commented Jan 25, 2021

Codecov Report

Merging #12229 (d2d9159) into master (9b5e66b) will decrease coverage by 14.12%.
The diff coverage is 18.70%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master   #12229       +/-   ##
===========================================
- Coverage   72.29%   58.17%   -14.13%     
===========================================
  Files         864      481      -383     
  Lines       44883    16120    -28763     
  Branches     5403     4124     -1279     
===========================================
- Hits        32450     9378    -23072     
+ Misses      12224     6742     -5482     
+ Partials      209        0      -209     
Flag Coverage Δ
cypress 58.17% <18.70%> (?)
javascript ?
python ?

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

Impacted Files Coverage Δ
...set-frontend/src/components/ListViewCard/index.tsx 100.00% <ø> (+5.45%) ⬆️
superset-frontend/src/components/Icons/index.ts 4.76% <4.76%> (ø)
...perset-frontend/src/views/CRUD/chart/ChartCard.tsx 70.83% <25.00%> (-1.39%) ⬇️
...rontend/src/views/CRUD/dashboard/DashboardCard.tsx 76.00% <50.00%> (+0.32%) ⬆️
...set-frontend/src/components/Icons/AntdEnhanced.tsx 75.00% <75.00%> (ø)
superset-frontend/src/components/Icons/Icon.tsx 100.00% <100.00%> (ø)
...perset-frontend/src/views/CRUD/chart/ChartList.tsx 80.18% <100.00%> (+8.52%) ⬆️
...rontend/src/views/CRUD/dashboard/DashboardList.tsx 72.63% <100.00%> (-1.25%) ⬇️
...tend/src/views/CRUD/data/database/DatabaseList.tsx 69.66% <100.00%> (-9.55%) ⬇️
...uperset-frontend/src/dashboard/util/dnd-reorder.js 0.00% <0.00%> (-100.00%) ⬇️
... and 780 more

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 9b5e66b...d2d9159. Read the comment docs.

@adam-stasiak
Copy link
Copy Markdown
Contributor

Tested manually across browsers and looks fine 🟢

@rusackas rusackas marked this pull request as draft January 26, 2021 17:52
@rusackas
Copy link
Copy Markdown
Member

For transparency, Diego and I spoke online, and we agreed to put this in Draft mode while exploring a couple options:

• Re-exporting vanilla AntD icons from the Superset Icon component, so we can get ALL icons from one import.
• Attempting to consolidate the interface for components, either with AntD's <WhateverIcon /> interface, or our own <Icon name="WhateverIcon" /> interface.
• Once the interface is conformed, trying to find a way to essentially override an AntD icon, which would allow a component developer to use AnyVanillaIcon from AntD, and then when we want to replace it with our own, we would simply override it in our Icon component, and not have to go around finding/replacing the scattered implementations.

Comment thread superset-frontend/src/components/ListView/ActionsBar.tsx
@rusackas rusackas marked this pull request as ready for review February 2, 2021 00:16
Copy link
Copy Markdown
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! This is a huge step forward! Will pull the branch and do a little more functional testing again before merging

@rusackas
Copy link
Copy Markdown
Member

rusackas commented Feb 2, 2021

This looks like it's ready to go, pending a teensy lint-fix

@rusackas
Copy link
Copy Markdown
Member

rusackas commented Feb 18, 2021

@junlincc @eschutho @nytai @ktmud @etr2460

With concerns of stability in mind, does anyone see any reason to further hesitate on merging this one in (after a rebase)? It looks like a very positive code quality change to me, so I'm getting an itchy trigger finger.

Copy link
Copy Markdown
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor nit - other than that LGTM and worked perfectly in my testing.

Comment thread superset-frontend/src/components/Icons/index.ts Outdated
@eschutho
Copy link
Copy Markdown
Member

cc @Steejay @mihir174
Looks great. Only question I have is whether we can we still use custom icons when designers want something new or different?

@rusackas
Copy link
Copy Markdown
Member

rusackas commented Feb 22, 2021

cc @Steejay @mihir174
Looks great. Only question I have is whether we can we still use custom icons when designers want something new or different?

Indeed! This is built specifically for that, so we can effectively extend/override the AntD icon set, rather than having custom icons that are a bit of an interface mismatch from the AntD icons we'd use elsewhere.

In fact, plans are underway to revamp our custom icons in the design system to better match the layout/sizing of the AntD icons. We'll get them all to play nicely together.

@rusackas rusackas closed this Feb 23, 2021
@rusackas rusackas reopened this Feb 23, 2021
@rusackas rusackas merged commit 8ab45c9 into apache:master Feb 25, 2021
allanco91 pushed a commit to allanco91/superset that referenced this pull request May 21, 2021
* Enhance custom icon

* Minor fix

* Move DashboardList icon trash icon to enhanced

* Enhance trash icon on lists

* Enhance actions icons card view

* Add storybook entry for custom icons

* Test delete button

* Remove commented line

* Fix linting issue

* Enhance Antd icons

* Enhance existing icons up to BoltSmallRunIcon

* Remove unused import

* Import/Exports all icons from index

* Export all existing icons

* Implement more enhanced icons

* Add data-id on edit buttons

* Fix lint issue

* Inherit color

* Apply original color to actions

* Fix linting issue

* Fix typo

* Change ModeHoriz to MoreHoriz
@mistercrunch 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
qfcwell pushed a commit to qfcwell/superset that referenced this pull request May 12, 2026
* Enhance custom icon

* Minor fix

* Move DashboardList icon trash icon to enhanced

* Enhance trash icon on lists

* Enhance actions icons card view

* Add storybook entry for custom icons

* Test delete button

* Remove commented line

* Fix linting issue

* Enhance Antd icons

* Enhance existing icons up to BoltSmallRunIcon

* Remove unused import

* Import/Exports all icons from index

* Export all existing icons

* Implement more enhanced icons

* Add data-id on edit buttons

* Fix lint issue

* Inherit color

* Apply original color to actions

* Fix linting issue

* Fix typo

* Change ModeHoriz to MoreHoriz
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/XL 🚢 1.2.0 First shipped in 1.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants