Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
+35 −11
Draft

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
998ddf0
style(Button): vertically align icons across all buttons and remove b…
EnxDev Jul 4, 2025
b86b972
Merge branch 'master' into enxdev/style/button
EnxDev Jul 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions superset-frontend/packages/superset-ui-core/src/components/Button/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
* under the License.
*/
import { Children, ReactElement, Fragment } from 'react';

import cx from 'classnames';
import { Button as AntdButton } from 'antd';
import { useTheme } from '@superset-ui/core';
Expand Down Expand Up @@ -90,7 +89,12 @@ export function Button(props: ButtonProps) {
const element = children as ReactElement;

let renderedChildren = [];
if (element && element.type === Fragment) {
if (
element &&
(element.type === Fragment ||
element.type === 'span' ||
element?.props?.css !== undefined)
) {
renderedChildren = Children.toArray(element.props.children);
} else {
Comment on lines +92 to 99

This comment was marked as resolved.

Sign in to view

renderedChildren = Children.toArray(children);
Expand Down Expand Up @@ -130,8 +134,9 @@ export function Button(props: ButtonProps) {
marginLeft: 0,
'& + .superset-button': {
marginLeft: theme.sizeUnit * 2,
boxShadow: 'unset',
},
'& > span > :first-of-type': {
'& svg': {
marginRight: firstChildMargin,
},
}}
Expand Down
1 change: 0 additions & 1 deletion superset-frontend/packages/superset-ui-core/src/components/Modal/Modal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,6 @@ export const StyledModal = styled(BaseModal)<StyledModalProps>`
.close {
flex: 1 1 auto;
margin-bottom: ${({ theme }) => theme.sizeUnit}px;
color: ${({ theme }) => theme.colorPrimaryText};
font-size: 32px;
font-weight: ${({ theme }) => theme.fontWeightLight};
}
Expand Down
1 change: 0 additions & 1 deletion superset-frontend/src/components/MessageToasts/Toast.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,6 @@ export default function Toast({ toast, onCloseToast }: ToastPresenterProps) {
<Icons.CloseOutlined
iconSize="m"
className="toast__close pointer"
iconColor={theme.colorTextTertiary}
role="button"
tabIndex={0}
onClick={handleClosePress}
Expand Down
11 changes: 9 additions & 2 deletions superset-frontend/src/pages/ChartList/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -774,10 +774,17 @@ function ChartList(props: ChartListProps) {
if (canCreate) {
subMenuButtons.push({
name: (
<>
<span
Copy link
Member

@mistercrunch mistercrunch Jul 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

style={{
display: 'inline-flex',
alignItems: 'center',
lineHeight: 0,
verticalAlign: 'middle',
}}
>
<Icons.PlusOutlined iconSize="m" />
<span>{t('Chart')}</span>
</>
</span>
),
buttonStyle: 'primary',
onClick: () => {
Expand Down
11 changes: 9 additions & 2 deletions superset-frontend/src/pages/DashboardList/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -695,10 +695,17 @@ function DashboardList(props: DashboardListProps) {
if (canCreate) {
subMenuButtons.push({
name: (
<>
<span
style={{
display: 'inline-flex',
alignItems: 'center',
lineHeight: 0,
verticalAlign: 'middle',
}}
>
<Icons.PlusOutlined iconSize="m" />
{t('Dashboard')}
</>
</span>
),
buttonStyle: 'primary',
onClick: () => {
Expand Down
11 changes: 9 additions & 2 deletions superset-frontend/src/pages/DatasetList/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -645,10 +645,17 @@ const DatasetList: FunctionComponent<DatasetListProps> = ({
if (canCreate) {
buttonArr.push({
name: (
<>
<span
style={{
display: 'inline-flex',
alignItems: 'center',
lineHeight: 0,
verticalAlign: 'middle',
}}
>
<Icons.PlusOutlined iconSize="m" />
{t('Dataset')}
</>
</span>
),
onClick: () => {
history.push('/dataset/add/');
Expand Down
Loading