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

Allow additional classes to be specified on the clean up button #1737

Merged
merged 7 commits into from
Aug 24, 2023
Merged

Allow additional classes to be specified on the clean up button #1737

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5d74004
Allow additional classes to be specified on the clean up button
rtorrero Aug 23, 2023
0cdbb13
Use existing 'classnames'
rtorrero Aug 23, 2023
a3bd998
Update storybook story
rtorrero Aug 23, 2023
b982d0d
Revert previous change and add size property
rtorrero Aug 24, 2023
609473f
Add 'fit' size to button
rtorrero Aug 24, 2023
826021d
Add size property to CleanUpButton
rtorrero Aug 24, 2023
e5ff559
Add button storybook changes
rtorrero Aug 24, 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
2 changes: 2 additions & 0 deletions assets/js/components/Button/Button.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ const getSizeClasses = (size) => {
switch (size) {
case 'small':
return 'py-1 px-2 text-sm';
case 'fit':
Copy link
Contributor

Choose a reason for hiding this comment

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

We could even add a story for this

return 'text-sm';
default:
return 'py-2 px-4 text-base';
}
Expand Down
15 changes: 15 additions & 0 deletions assets/js/components/Button/Button.stories.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,17 @@ import Button from '.';
export default {
title: 'Button',
component: Button,
argTypes: {
size: {
control: { type: 'radio' },
options: ['small', 'fit'],
description: 'Button size',
table: {
type: { summary: 'string' },
defaultValue: { summary: 'small' },
},
},
},
};

export function Default() {
Expand All @@ -27,6 +38,10 @@ export function Small() {
return <Button size="small">Hello world!</Button>;
}

export function Fit() {
return <Button size="fit">Hello world!</Button>;
}

export function SmallSecondary() {
return (
<Button size="small" type="secondary">
Expand Down
12 changes: 9 additions & 3 deletions assets/js/components/CleanUpButton/CleanUpButton.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,22 @@
import React from 'react';

import { EOS_CLEANING_SERVICES } from 'eos-icons-react';
import classNames from 'classnames';

import Button from '@components/Button';
import Spinner from '@components/Spinner';

function CleanUpButton({ cleaning, onClick }) {
function CleanUpButton({ className, size = 'small', cleaning, onClick }) {
const buttonClasses = classNames(
'inline-block mx-0.5 border-green-500 border w-fit',
className
);

return (
<Button
type="primary-white"
className="inline-block mx-0.5 border-green-500 border w-fit"
size="small"
className={buttonClasses}
size={size}
disabled={cleaning}
onClick={() => onClick()}
Copy link
Member

Choose a reason for hiding this comment

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

Just a comment, when we have this "dummy" components, without any particular logic on the event handlers, for example take a modal which has on the onClose prop the id of something passed like onClose(id), that is a complex handler.

In that button we have a direct onclick handler, without any logic and it's a common pratice, but totally optional, to pass directly the handler, like onClick={onClick}, so the entire event chain of the dom event is passed up to the parent. The parent can listen if want to the event as first argument of the onClick prop callback.

This is completely optional in that case, there are different opinions about that, for sure it's true in pure design systems component, take bootstrap for example, you want to give the callee the direct facade to the html event.

If you want change it, otherwise leave as is and we could have this comment as a future discussion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, now that I read this, I'm wondering how it works (JS mysteries to me XD).
Because in the component, we have () => onClick(), but in the call to the component we have:

onClick={() => {
    openDeregistrationModal(item);
    openDeregistrationModal(item);
 }}

so it is kind of done twice.
Sorry for my JS dummy questions hehe
Maybe it is totally normal

>
Expand Down
24 changes: 24 additions & 0 deletions assets/js/components/CleanUpButton/CleanUpButton.stories.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,22 @@ export default {
},
},
onClick: { action: 'Click button' },
className: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add size to the options

control: 'text',
description: 'CSS classes',
table: {
type: { summary: 'string' },
},
},
size: {
control: { type: 'radio' },
options: ['small', 'fit'],
description: 'Button size',
table: {
type: { summary: 'string' },
defaultValue: { summary: 'small' },
},
},
},
};

Expand All @@ -26,3 +42,11 @@ export const Cleaning = {
cleaning: true,
},
};

export const NoOutline = {
args: {
...Default.args,
className: 'border-none shadow-none',
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we put size: 'fit' as the example

size: 'fit',
},
};
2 changes: 2 additions & 0 deletions assets/js/components/HostsList.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,8 @@ function HostsList() {
content && (
<CleanUpButton
cleaning={item.deregistering}
size="fit"
className="border-none shadow-none"
onClick={() => {
openDeregistrationModal(item);
}}
Expand Down