style(Button): Vertically align icons across all buttons and remove box shadow

[EnxDev]

SUMMARY

This PR improves the visual consistency of buttons across the UI by vertically aligning icons within buttons and removing the default box shadow.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

• Before
  

• After
  

• Before
  

• After
  

• Before
  

• After
  

• Before
  

• After
  

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[korbit-ai]

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.

Category	Issue	Status
	Unsafe Children Property Access ▹ view	🧠 Incorrect

Files scanned

​

File Path	Reviewed
superset-frontend/src/components/MessageToasts/Toast.tsx	✅
superset-frontend/packages/superset-ui-core/src/components/Button/index.tsx	✅
superset-frontend/packages/superset-ui-core/src/components/Modal/Modal.tsx	✅
superset-frontend/src/pages/DashboardList/index.tsx	✅
superset-frontend/src/pages/ChartList/index.tsx	✅
superset-frontend/src/pages/DatasetList/index.tsx	✅

Explore our documentation to understand the languages and file types we support and the files we ignore.

Check out our docs on how you can make Korbit work best for you and your team.

Loving Korbit!? Share us on LinkedIn Reddit and X

[mistercrunch]

Mmmh, does antd gets this wrong, or is it because we're not using antd right?

I noticed antd's button expects an icon prop, but often we pass the icon as a children instead. Is that the case that what we're addressing here?

If it's the case I'd much rather fix our use of <Button icon={} /> throughout the codebase than patching it with styles in the wrapper.

From my understanding, if/when using button with the icon prop, antd should do all the right things in terms of alignement with standard sizing and all. Guessing it may pass-in/override the icon sizing based on the button's size attributes, also probably should get the color of the icon to match the text and all. I've seen places where we theme.colors.warning.light4 on the icon to try and match the text's color and that's just stuff antd will do properly when used right ....

[mistercrunch]

Here it seems that we could either change the SubMenu components to receive actual buttons directly (instead of data structures that get turned into buttons (not sure why someone created this indirection?)), or you could extend it to expect receiving a icon key in as part of the object.

Seems passing in an array of buttons is more reasonable, but looks like SubMenu is used in many places:

12:06 $ git grep "<SubMenu"
superset-frontend/packages/superset-ui-core/src/ui-overrides/types.ts:  'home.submenu': ComponentType<SubMenuProps>;
superset-frontend/src/features/home/ActivityTable.tsx:      <SubMenu
superset-frontend/src/features/home/ChartTable.tsx:      <SubMenu
superset-frontend/src/features/home/DashboardTable.tsx:      <SubMenu
superset-frontend/src/features/home/LanguagePicker.tsx:    <SubMenu
superset-frontend/src/features/home/SavedQueries.tsx:      <SubMenu
superset-frontend/src/features/home/SubMenu.test.tsx:      <SubMenu {...props} />
superset-frontend/src/features/home/SubMenu.tsx:const SubMenuComponent: FunctionComponent<SubMenuProps> = props => {
superset-frontend/src/features/home/SubMenu.tsx:              <SubMenu
superset-frontend/src/pages/ActionLog/index.tsx:      <SubMenu name={t('Action Logs')} buttons={subMenuButtons} />
superset-frontend/src/pages/AlertReportList/index.tsx:      <SubMenu
superset-frontend/src/pages/AnnotationLayerList/index.tsx:      <SubMenu name={t('Annotation layers')} buttons={subMenuButtons} />
superset-frontend/src/pages/AnnotationList/index.tsx:      <SubMenu
superset-frontend/src/pages/ChartList/index.tsx:      <SubMenu name={t('Charts')} buttons={subMenuButtons} />
superset-frontend/src/pages/CssTemplateList/index.tsx:      <SubMenu {...menuData} />
superset-frontend/src/pages/DashboardList/index.tsx:      <SubMenu name={t('Dashboards')} buttons={subMenuButtons} />
superset-frontend/src/pages/DatabaseList/index.tsx:      <SubMenu {...menuData} />
superset-frontend/src/pages/DatasetList/index.tsx:      <SubMenu {...menuData} />
superset-frontend/src/pages/ExecutionLogList/index.tsx:      <SubMenu
superset-frontend/src/pages/GroupsList/index.tsx:      <SubMenu name={t('List Groups')} buttons={subMenuButtons} />
superset-frontend/src/pages/Home/index.tsx:        <SubMenu {...menuData} />
superset-frontend/src/pages/QueryHistoryList/index.tsx:      <SubMenu {...menuData} />
superset-frontend/src/pages/RolesList/index.tsx:      <SubMenu name={t('List Roles')} buttons={subMenuButtons} />
superset-frontend/src/pages/RowLevelSecurityList/index.tsx:      <SubMenu name={t('Row Level Security')} buttons={subMenuButtons} />
superset-frontend/src/pages/SavedQueryList/index.tsx:      <SubMenu {...menuData} />
superset-frontend/src/pages/Tags/index.tsx:      <SubMenu name={t('Tags')} buttons={subMenuButtons} />
superset-frontend/src/pages/UserInfo/index.tsx:      <SubMenu buttons={SubMenuButtons} />
superset-frontend/src/pages/UserRegistrations/index.tsx:      <SubMenu name={t('User registrations')} />
superset-frontend/src/pages/UsersList/index.tsx:      <SubMenu name={t('List Users')} buttons={subMenuButtons} />

[mistercrunch]

Personally I'm in favor of refactoring/fixing over adding yet another layer on top.

[mistercrunch]

about box-shadow, I'd vote for keeping Superset vanilla antd, and if we decide to change the default style of buttons to not have box-shadow, I'd do it in the default's theme definition, no in the component.

By unsetting box-shadow in the Button component here, I think you're preventing anyone for having any shadow while they'd be trying to add a shadow by specifying they want shadow in their theme.