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.

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

[EuiButton][EuiButtonEmpty][EuiButtonIcon] Remove color="ghost" #7296

Merged
merged 9 commits into from
Oct 25, 2023
+137 −373
Merged

[EuiButton][EuiButtonEmpty][EuiButtonIcon] Remove color="ghost" #7296

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e7c29fd
Remove `ghost` color from EUI buttons
cee-chen Oct 18, 2023
b7dc9b4
Clean up props
cee-chen Oct 18, 2023
b5e5a8c
[docs] Remove `ghost` button documentation
cee-chen Oct 18, 2023
be0c695
[docs] Fix other `color="ghost"` button usages
cee-chen Oct 18, 2023
76577f7
[docs] Fix broken sticky bottom bar demo
cee-chen Oct 18, 2023
b95716e
[EuiControlBar] Remove `ghost` colors
cee-chen Oct 18, 2023
d54b47b
changelog
cee-chen Oct 18, 2023
5c871b2
Update mounted snapshot changes caused by `defaultProps` removal
cee-chen Oct 18, 2023
6d6d79b
[docs] Fix `color` prop not correctly showing the default value
cee-chen Oct 18, 2023
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
27 changes: 16 additions & 11 deletions src-docs/src/components/guide_theme_selector/guide_theme_selector.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
/* eslint-disable no-restricted-globals */
import React, { useState } from 'react';

import { useIsWithinBreakpoints } from '../../../../src/services';
import {
EuiThemeProvider,
useIsWithinBreakpoints,
} from '../../../../src/services';
import { EUI_THEME, EUI_THEMES } from '../../../../src/themes';

import { ThemeContext } from '../with_theme';
Expand Down Expand Up @@ -84,16 +87,18 @@ const GuideThemeSelectorComponent: React.FunctionComponent<
});

const button = (
<EuiButton
size="s"
iconType="arrowDown"
iconSide="right"
color="ghost"
minWidth={0}
onClick={onButtonClick}
>
{isMobileSize ? 'Theme' : currentTheme.text}
</EuiButton>
<EuiThemeProvider colorMode="dark" wrapperProps={{ cloneElement: true }}>
Copy link
Member

Choose a reason for hiding this comment

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

Looking at the Update Paths, this would be a case where:

if not, create your own wrapper around your button, and then change to color="text"

So in this context, we want to maintain the "ghost" coloring of this button, which is why we wrap it in a theme provider set explicitly to "dark".

This essentially makes the button permanently and independently exist in a "dark mode" context, isolating it from any other dark/light mode changes in any higher-level theme contexts.

Does that all track? I'm just trying to understand.

Does this only impact color mode? Meaning, if other theme changes are set in a higher-level theme context, would they still flow through to this button?

Copy link
Contributor Author

@cee-chen cee-chen Oct 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This essentially makes the button permanently and independently exist in a "dark mode" context, isolating it from any other dark/light mode changes in any higher-level theme contexts.

Yeah, your understanding is totally correct!

Does this only impact color mode? Meaning, if other theme changes are set in a higher-level theme context, would they still flow through to this button?

Yes! Child theme providers will always inherit from their most recent parent. So you could even do something like this:

<EuiProvider>
  // App

  <EuiThemeProvider modify={{ size: { base: 20 } }}>
    // Some modified content
    <EuiThemeProvider colorMode="dark">
      // Will inherit *both* the size override and dark color mode settings
    </EuiThemeProvider>
  </EuiThemeProvider>
</EuiProvider>

Here's the source code for that behavior if you're curious (we grab the parent React context and then merge in any modifications and then pass that down as the new context):

eui/src/services/theme/provider.tsx

Lines 74 to 77 in b1c8582

const parentSystem = useContext(EuiSystemContext);
const parentModifications = useContext(EuiModificationsContext);
const parentColorMode = useContext(EuiColorModeContext);
const parentTheme = useContext(EuiThemeContext);

All the credit to Greg for creating that component with flexibility and future-proof-ness in mind!

<EuiButton
size="s"
iconType="arrowDown"
iconSide="right"
color="text"
minWidth={0}
onClick={onButtonClick}
>
{isMobileSize ? 'Theme' : currentTheme.text}
</EuiButton>
</EuiThemeProvider>
);

return (
Expand Down
18 changes: 8 additions & 10 deletions src-docs/src/views/bottom_bar/bottom_bar_position.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,25 +1,23 @@
import React from 'react';

import { EuiBottomBar, EuiSpacer, EuiText } from '../../../../src/components';
import { EuiBottomBar, EuiText } from '../../../../src/components';

export default () => {
return (
<>
<div css={{ overflow: 'auto', blockSize: 200 }}>
Copy link
Member

Choose a reason for hiding this comment

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

This is just a bonus fix in addition to the ghost changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! It's in its own separate commit as well 76577f7. I usually add a note to my PR descriptions recommending that folks follow along with changes by commit as I tend to throw in a lot of misc cleanup items in (but I try really hard to keep my git history clean so that optional cleanup items are separate from actual features or main functionality).

TBH I noticed a while back that the demo was broken on prod and was too lazy to fix it then, this PR was a good excuse 😅

<EuiText>
<p>
When scrolling past this example block, the{' '}
<strong>EuiBottomBar</strong> will stick to the bottom of the browser
window (with a 10px offset), but keeps it within the bounds of its
parent.
When scrolling within this example, the <strong>EuiBottomBar</strong>{' '}
will stick to the bottom of scrollable container (with a 10px offset),
but will not scroll with the page itself.
</p>
</EuiText>
<EuiSpacer size="xl" />
<EuiSpacer size="xl" />
<div css={{ blockSize: 400 }} />
<EuiBottomBar position="sticky" bottom={10}>
<EuiText color="ghost" textAlign="center">
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't we need color="text" here to maintain the ghost coloring?

Copy link
Contributor Author

@cee-chen cee-chen Oct 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

EuiText defaults to color="text"! So we don't need to specify a color (unlike buttons which default to color="primary")

Edit to add: And EuiBottomBar already sets a dark mode context by default

<EuiText textAlign="center">
<p>Scroll to see!</p>
</EuiText>
</EuiBottomBar>
</>
</div>
);
};
35 changes: 0 additions & 35 deletions src-docs/src/views/button/button_example.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,12 +101,6 @@ const splitButtonSnippet = [
</EuiFlexGroup>`,
];

import ButtonGhost from './button_ghost';
const buttonGhostSource = require('!!raw-loader!./button_ghost');
const buttonGhostSnippet = `<EuiButton color="ghost">
<!-- Button text -->
</EuiButton>`;

import ButtonAsLink from './button_as_link';
const buttonAsLinkSource = require('!!raw-loader!./button_as_link');
const buttonAsLinkSnippet = `<EuiButton href={someUrl}><!-- Button text --></EuiButton>
Expand Down Expand Up @@ -703,35 +697,6 @@ export const ButtonExample = {
props: { EuiButtonGroup, EuiButtonGroupOptionProps },
demoPanelProps: { color: 'subdued' },
},
{
title: 'Ghost vs. dark mode',
source: [
{
type: GuideSectionTypes.JS,
code: buttonGhostSource,
},
],
text: (
<p>
For buttons on dark color backgrounds, it is recommended to wrap the
entire area in{' '}
<EuiCode language="js">
{'<EuiThemeProvider colorMode="dark">'}
</EuiCode>{' '}
to invert all colors to dark mode. The legacy{' '}
<EuiCode language="js">{'color="ghost"'}</EuiCode> now uses this
method and is set for deprecation. An example usage of inverted
buttons is in combination with{' '}
<Link to="/layout/bottom-bar">
<strong>EuiBottomBar</strong>
</Link>{' '}
which already wraps its children in dark mode.
</p>
),
snippet: buttonGhostSnippet,
demo: <ButtonGhost />,
demoPanelProps: { paddingSize: 'none' },
},
],
guidelines: <Guidelines />,
};
84 changes: 0 additions & 84 deletions src-docs/src/views/button/button_ghost.js
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion src-docs/src/views/page_template/_page_demo.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ export const PageDemo: FunctionComponent<
</EuiText>
</EuiFlexItem>
<EuiFlexItem grow={false}>
<EuiButton color="ghost">Share</EuiButton>
<EuiButton color="text">Share</EuiButton>
</EuiFlexItem>
</EuiFlexGroup>
) : undefined;
Expand Down
11 changes: 0 additions & 11 deletions src/components/button/__snapshots__/button.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,17 +41,6 @@ exports[`EuiButton props color danger is rendered 1`] = `
</button>
`;

exports[`EuiButton props color ghost is rendered 1`] = `
<button
class="euiButton emotion-euiButtonDisplay-m-defaultMinWidth-base-text-euiColorMode-dark"
type="button"
>
<span
class="emotion-euiButtonDisplayContent"
/>
</button>
`;

exports[`EuiButton props color primary is rendered 1`] = `
<button
class="euiButton emotion-euiButtonDisplay-m-defaultMinWidth-base-primary"
Expand Down
47 changes: 13 additions & 34 deletions src/components/button/button.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,9 @@ import {
EuiButtonDisplayCommonProps,
isButtonDisabled,
} from './button_display/_button_display';
import { EuiThemeProvider } from '../../services';

export const COLORS = [...BUTTON_COLORS, 'ghost'] as const;
export type EuiButtonColor = _EuiButtonColor | 'ghost';
export const COLORS = BUTTON_COLORS;
export type EuiButtonColor = _EuiButtonColor;

export const SIZES = ['s', 'm'] as const;
export type EuiButtonSize = (typeof SIZES)[number];
Expand All @@ -43,7 +42,6 @@ interface BaseProps {
fill?: boolean;
/**
* Any of the named color palette options.
* **`'ghost'` is set for deprecation. Use EuiThemeProvide.colorMode = 'dark' instead.**
*/
color?: EuiButtonColor;
/**
Expand Down Expand Up @@ -84,55 +82,36 @@ export type Props = ExclusiveUnion<
* EuiButton is largely responsible for providing relevant props
* and the logic for element-specific attributes
*/
export const EuiButton: FunctionComponent<Props> = (props) => {
const {
className,
buttonRef,
color: _color = 'primary',
fill,
...rest
} = props;

const buttonIsDisabled = isButtonDisabled({
export const EuiButton: FunctionComponent<Props> = ({
className,
buttonRef,
size = 'm',
color = 'primary',
fill,
...rest
}) => {
const isDisabled = isButtonDisabled({
href: rest.href,
isDisabled: rest.isDisabled || rest.disabled,
isLoading: rest.isLoading,
});

const color = buttonIsDisabled ? 'disabled' : _color;

const buttonColorStyles = useEuiButtonColorCSS({
display: fill ? 'fill' : 'base',
})[color === 'ghost' ? 'text' : color];
})[isDisabled ? 'disabled' : color];

const buttonFocusStyle = useEuiButtonFocusCSS();

const classes = classNames('euiButton', className);
const cssStyles = [buttonColorStyles, buttonFocusStyle];

if (_color === 'ghost') {
// INCEPTION: If `ghost`, re-implement with a wrapping dark mode theme provider
return (
<EuiThemeProvider colorMode="dark" wrapperProps={{ cloneElement: true }}>
<EuiButton {...props} color="text" />
</EuiThemeProvider>
);
}

return (
<EuiButtonDisplay
className={classes}
css={cssStyles}
ref={buttonRef}
size={size}
{...rest}
/>
);
};

EuiButton.displayName = 'EuiButton';
Copy link
Member

Choose a reason for hiding this comment

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

I take it this was just redundant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, unless we're using forwardRef or have some other reason for renaming components (e.g. HOCs and other shenanigans), we don't need to manually set a displayName.


// Use defaultProps for simple pass-through props
EuiButton.defaultProps = {
size: 'm',
color: 'primary',
};
15 changes: 0 additions & 15 deletions src/components/button/button_empty/__snapshots__/button_empty.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,21 +49,6 @@ exports[`EuiButtonEmpty props color danger is rendered 1`] = `
</button>
`;

exports[`EuiButtonEmpty props color ghost is rendered 1`] = `
<button
class="euiButtonEmpty emotion-euiButtonDisplay-euiButtonEmpty-m-empty-text-euiColorMode-dark"
type="button"
>
<span
class="euiButtonEmpty__content emotion-euiButtonDisplayContent"
>
<span
class="eui-textTruncate euiButtonEmpty__text"
/>
</span>
</button>
`;

exports[`EuiButtonEmpty props color primary is rendered 1`] = `
<button
class="euiButtonEmpty emotion-euiButtonDisplay-euiButtonEmpty-m-empty-primary"
Expand Down
6 changes: 0 additions & 6 deletions src/components/button/button_empty/button_empty.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,12 +88,6 @@ describe('EuiButtonEmpty', () => {
expect(container.firstChild).toMatchSnapshot();
});
});

test('ghost is rendered', () => {
const { container } = render(<EuiButtonEmpty color={'ghost'} />);

expect(container.firstChild).toMatchSnapshot();
});
});

describe('size', () => {
Expand Down
Loading