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

ToolTip: Remove knobs in stories #46515

Merged
merged 1 commit into from
Dec 21, 2022
Merged

ToolTip: Remove knobs in stories #46515

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
146 changes: 68 additions & 78 deletions packages/components/src/tooltip/stories/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,3 @@
/**
* External dependencies
*/
import styled from '@emotion/styled';
import { text, select, number } from '@storybook/addon-knobs';

/**
* WordPress dependencies
*/
import { useState } from '@wordpress/element';

/**
* Internal dependencies
*/
Expand All @@ -17,79 +6,80 @@ import Tooltip from '../';
export default {
title: 'Components/ToolTip',
component: Tooltip,
argTypes: {
delay: { control: 'number' },
Copy link
Member

Choose a reason for hiding this comment

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

Should we also add a defaultValue of 700 to keep the default value that was there when we used knobs?

Suggested change
delay: { control: 'number' },
delay: { control: 'number', defaultValue: 700 },

Feel free to place it in Default.args if that makes more sense for you.

Copy link
Member Author

Choose a reason for hiding this comment

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

In general we've been taking the direction of not re-specifying default values in Storybook, so we can showcase the actual defaults that are coming from the component code.

The props table will show descriptions and default values once this is converted to TypeScript, which will also help.

position: {
Copy link
Member

Choose a reason for hiding this comment

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

Should we default to top center as we did for the previous version of the story that used knobs?

Suggested change
position: {
position: {
defaultValue: 'top center',

Copy link
Member Author

Choose a reason for hiding this comment

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

Same here.

control: {
type: 'select',
options: [
'top left',
'top center',
'top right',
'bottom left',
'bottom center',
'bottom right',
],
},
},
text: { control: 'text' },
},
parameters: {
knobs: { disable: false },
docs: { source: { state: 'open' } },
},
};

export const _default = () => {
const positionOptions = {
'top left': 'top left',
'top center ': 'top center',
'top right': 'top right',
'bottom left': 'bottom left',
'bottom center ': 'bottom center',
'bottom right': 'bottom right',
};
const tooltipText = text( 'Text', 'More information' );
const position = select( 'Position', positionOptions, 'top center' );
const delay = number( 'Delay', 700 );
return (
<>
<Tooltip text={ tooltipText } position={ position } delay={ delay }>
<div
style={ {
margin: '50px auto',
width: '200px',
padding: '20px',
textAlign: 'center',
border: '1px solid #ccc',
} }
>
Hover for more information
</div>
</Tooltip>
<Tooltip text={ tooltipText } position={ position } delay={ delay }>
<div
style={ {
margin: '50px auto',
width: 'min-content',
padding: '4px',
textAlign: 'center',
border: '1px solid #ccc',
} }
>
Small target
</div>
</Tooltip>
</>
);
const Template = ( args ) => {
return <Tooltip { ...args } />;
};

const Button = styled.button`
margin: 0 10px;
`;
export const Default = Template.bind( {} );
Default.args = {
text: 'More information',
children: (
<div
style={ {
margin: '50px auto',
width: '200px',
padding: '20px',
textAlign: 'center',
border: '1px solid #ccc',
} }
>
Hover for more information
</div>
),
};

export const DisabledElement = () => {
const [ showMessage, toggleMessage ] = useState( false );
export const SmallTarget = Template.bind( {} );
SmallTarget.args = {
...Default.args,
children: (
<div
style={ {
margin: '50px auto',
width: 'min-content',
padding: '4px',
textAlign: 'center',
border: '1px solid #ccc',
} }
>
Small target
</div>
),
};

return (
<>
<Tooltip text="Hey, I am tooltip" position="bottom center">
<Button onClick={ () => toggleMessage( ! showMessage ) }>
Hover me!
</Button>
</Tooltip>
<Tooltip text="Hey, I am tooltip" position="bottom center">
<Button
disabled
onClick={ () => toggleMessage( ! showMessage ) }
>
Hover me, but I am disabled
</Button>
</Tooltip>
<br />
{ showMessage ? <p>Hello World!</p> : null }
</>
);
export const DisabledChild = Template.bind( {} );
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this story can be removed because it's basically just a mock for manual testing (that is already covered in unit tests #35254) 🤔 I'm fine with leaving it in for now though.

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with leaving it as well, I guess someone could find it useful.

DisabledChild.args = {
...Default.args,
children: (
<button
disabled
onClick={ () =>
// eslint-disable-next-line no-alert
window.alert( 'This alert should not be triggered' )
}
>
Hover me, but I am disabled
</button>
),
};