Skip to content
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.

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

fix: add right margin to <Button kind='inline'/> using icons #1664

Merged
merged 3 commits into from
Dec 4, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
45 changes: 21 additions & 24 deletions packages/code-studio/src/styleguide/Buttons.tsx
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
/* eslint-disable react/jsx-props-no-spreading */
import React, { Component, ReactElement } from 'react';
import classNames from 'classnames';
import { FontAwesomeIcon } from '@fortawesome/react-fontawesome';
import { ButtonOld, SocketedButton } from '@deephaven/components';
import { Button, ButtonOld, SocketedButton } from '@deephaven/components';
import { dhTruck } from '@deephaven/icons';
import { sampleSectionIdAndClasses } from './utils';

Expand Down Expand Up @@ -95,37 +93,36 @@ class Buttons extends Component<Record<string, never>, ButtonsState> {
>
<h5>Inline Buttons</h5>
Regular btn-inline:
<ButtonOld className="btn-inline mx-2">
<FontAwesomeIcon icon={dhTruck} />
</ButtonOld>
<Button className="mx-2" kind="inline" icon={dhTruck} tooltip="test" />
Toggle button (class active):
<ButtonOld
className={classNames('btn-inline mx-2', { active: toggle })}
<Button
className="mx-2"
onClick={() => {
this.setState({ toggle: !toggle });
}}
>
<FontAwesomeIcon icon={dhTruck} />
</ButtonOld>
active={toggle}
kind="inline"
icon={dhTruck}
tooltip="test"
/>
Disabled:
<ButtonOld className="btn-inline mx-2" disabled>
<FontAwesomeIcon icon={dhTruck} />
</ButtonOld>
<Button className="mx-2" kind="inline" disabled>
Disabled
</Button>
With Text:
<Button className="mx-2" kind="inline" icon={dhTruck}>
<span>Text Button</span>
</Button>
<br />
<br />
<span>btn-link-icon (no text):</span>
<ButtonOld className="btn-link btn-link-icon px-2">
{/* pad and margin horizontally as appropriate for icon shape and spacing,
needs btn-link and btn-link-icon classes. */}
<FontAwesomeIcon icon={dhTruck} />
</ButtonOld>
<Button kind="ghost" icon={dhTruck} tooltip="test" />
<span className="mx-2">btn-link:</span>
<ButtonOld className="btn-link">Text Button</ButtonOld>
<Button kind="ghost">Text Button </Button>
<span className="mx-2">btn-link (text w/ optional with icon):</span>
<ButtonOld className="btn-link">
<FontAwesomeIcon icon={dhTruck} />
Add Item
</ButtonOld>
<Button kind="ghost" icon={dhTruck}>
Text Button
</Button>
</div>
);
}
Expand Down
28 changes: 26 additions & 2 deletions packages/components/src/Button.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,30 @@ const Button = React.forwardRef<HTMLButtonElement, ButtonProps>(
);
}

// Best effort backwards-compatible attempt to auto add margin to icon if text is also present
// We would need to audit our usage of Buttons to remove margins by classname to just add the margin to every icon button with children
if (iconElem != null && children != null) {
// check if react children contains a text node to a depth of 2
// to exlude poppers/menus, but not button text nested in spans
let containsTextNode = false;
React.Children.forEach(children, child => {
if (typeof child === 'string') {
containsTextNode = true;
} else if (React.isValidElement(child)) {
React.Children.forEach(child.props.children, grandchild => {
if (typeof grandchild === 'string') {
containsTextNode = true;
}
});
}
});
if (containsTextNode) {
iconElem = React.cloneElement(iconElem, {
className: classNames('mr-1', iconElem.props.className),
});
}
}

let tooltipElem: JSX.Element | undefined;
if (tooltip !== undefined) {
tooltipElem =
Expand Down Expand Up @@ -180,10 +204,10 @@ const Button = React.forwardRef<HTMLButtonElement, ButtonProps>(
// disabled buttons tooltips need a wrapped element to receive pointer events
// https://jakearchibald.com/2017/events-and-disabled-form-fields/

return disabled ? (
return disabled && tooltip != null ? (
<span className="btn-disabled-wrapper">
{button}
{tooltip !== undefined && tooltipElem}
{tooltipElem}
</span>
) : (
button
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified tests/styleguide.spec.ts-snapshots/icons-chromium-linux.png
Binary file modified tests/styleguide.spec.ts-snapshots/icons-firefox-linux.png
Binary file modified tests/styleguide.spec.ts-snapshots/icons-webkit-linux.png
Loading