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

rtorrero · 2023-08-23T09:13:39Z

Description

This PR:

Allows the CleanUpButton component to be provided with additional classes
Changes the HostList to remove the outlining of the Clean Up button by providing additional classes

This is also needed for the upcoming PR to include the Clean Up button in the sap system instances

arbulu89

Could you add a new story book entry with the outlined example?

arbulu89 · 2023-08-23T09:17:42Z

assets/js/components/CleanUpButton/CleanUpButton.jsx

  return (
    <Button
      type="primary-white"
-      className="inline-block mx-0.5 border-green-500 border w-fit"
+      className={`${baseClasses} ${className}`}


Can we use classNames?
import classNames from 'classnames';

arbulu89 · 2023-08-23T09:18:50Z

assets/js/components/CleanUpButton/CleanUpButton.jsx

@@ -5,11 +5,13 @@ import { EOS_CLEANING_SERVICES } from 'eos-icons-react';
 import Button from '@components/Button';
 import Spinner from '@components/Spinner';

-function CleanUpButton({ cleaning, onClick }) {
+function CleanUpButton({ cleaning, onClick, className = '' }) {


You don't need to default to empty string I think.
I would work without default as well

arbulu89

Thank you!
I don't know if @jagabomb wants to have a look before merging

CDimonaco

Good job, LGTM

The comment is completely up to you, we can leave it for future reference and discussion, feel free to merge :D

CDimonaco · 2023-08-24T10:00:28Z

assets/js/components/CleanUpButton/CleanUpButton.jsx

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


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.

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

arbulu89

@rtorrero @jagabomb
I have to confess that for me this implentation is a step back.
I think that we could achieve the same keeping the button using some styles.
If using the link is mandatory, i would create other component. This if thing looks a bad implementation.
It doesn't have the spinning state neither

Don't like the new implementation

jagabomb · 2023-08-24T13:31:33Z

@rtorrero @jagabomb
I have to confess that for me this implentation is a step back.
I think that we could achieve the same keeping the button using some styles.
If using the link is mandatory, i would create other component. This if thing looks a bad implementation.
It doesn't have the spinning state neither

@arbulu89 I think you might be right, maybe using the Button component is overkill and maybe we need a simpler TextLink component which does the same thing as a the Button component but removes the additional padding? Just a thought from my side.

CDimonaco · 2023-08-24T13:34:45Z

@arbulu89 I'm with you, I approved before but the new implementation with if, looks a little bit clumsy

rtorrero · 2023-08-24T14:22:16Z

@jagabomb i've updated the component to add a variant with a size setting that allows to fit the button to the available size. If this still continues to be an issue, do you mind if we solve it in a separate PR to avoid stalling this? I need this component to submit the next PR and I think there we will be able to show how it looks and if the issue is a problem. WDYT?

PS: I can later add an additional component, something like but I'd need a bit of time to do a better implementation

jagabomb · 2023-08-24T14:31:18Z

@jagabomb i've updated the component to add a variant with a size setting that allows to fit the button to the available size. If this still continues to be an issue, do you mind if we solve it in a separate PR to avoid stalling this? I need this component to submit the next PR and I think there we will be able to show how it looks and if the issue is a problem. WDYT?

PS: I can later add an additional component, something like but I'd need a bit of time to do a better implementation

@rtorrero I am happy for us to proceed. My feedback is all proactive so it can always be addressed later.

arbulu89 · 2023-08-24T14:26:33Z

assets/js/components/CleanUpButton/CleanUpButton.stories.jsx

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


Could we put size: 'fit' as the example

arbulu89 · 2023-08-24T14:26:53Z

assets/js/components/CleanUpButton/CleanUpButton.stories.jsx

@@ -13,6 +13,13 @@ export default {
      },
    },
    onClick: { action: 'Click button' },
+    className: {


Add size to the options

arbulu89 · 2023-08-24T14:31:19Z

assets/js/components/CleanUpButton/CleanUpButton.jsx

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


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

jagabomb

LGTM, feedback will be addressed later.

arbulu89

Good to go from my side

arbulu89 · 2023-08-24T14:37:35Z

assets/js/components/Button/Button.jsx

@@ -5,6 +5,8 @@ const getSizeClasses = (size) => {
  switch (size) {
    case 'small':
      return 'py-1 px-2 text-sm';
+    case 'fit':


We could even add a story for this

* Allow additional classes to be specified on the clean up button * Add 'fit' size to button * Add size property to CleanUpButton

Allow additional classes to be specified on the clean up button

5d74004

rtorrero added ux javascript Pull requests that update Javascript code labels Aug 23, 2023

rtorrero requested review from dottorblaster, arbulu89 and jagabomb August 23, 2023 09:13

arbulu89 added the enhancement New feature or request label Aug 23, 2023

arbulu89 reviewed Aug 23, 2023

View reviewed changes

rtorrero added 2 commits August 23, 2023 11:32

Use existing 'classnames'

0cdbb13

Update storybook story

a3bd998

rtorrero requested a review from arbulu89 August 23, 2023 10:34

arbulu89 previously approved these changes Aug 23, 2023

View reviewed changes

CDimonaco approved these changes Aug 24, 2023

View reviewed changes

arbulu89 reviewed Aug 24, 2023

View reviewed changes

CDimonaco self-requested a review August 24, 2023 13:34

Revert previous change and add size property

b982d0d

rtorrero force-pushed the cleanup-no-border branch from a465922 to b982d0d Compare August 24, 2023 14:20

arbulu89 reviewed Aug 24, 2023

View reviewed changes

jagabomb approved these changes Aug 24, 2023

View reviewed changes

rtorrero added 2 commits August 24, 2023 15:35

Add 'fit' size to button

609473f

Add size property to CleanUpButton

826021d

arbulu89 approved these changes Aug 24, 2023

View reviewed changes

Add button storybook changes

e5ff559

rtorrero merged commit 5a74ca7 into main Aug 24, 2023

rtorrero deleted the cleanup-no-border branch August 24, 2023 14:47

EMaksy pushed a commit that referenced this pull request Aug 28, 2023

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

210229b

* Allow additional classes to be specified on the clean up button * Add 'fit' size to button * Add size property to CleanUpButton

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

rtorrero commented Aug 23, 2023

arbulu89 left a comment

arbulu89 Aug 23, 2023

arbulu89 Aug 23, 2023

arbulu89 left a comment

CDimonaco left a comment

CDimonaco Aug 24, 2023

arbulu89 Aug 24, 2023

arbulu89 left a comment

jagabomb commented Aug 24, 2023

CDimonaco commented Aug 24, 2023

rtorrero commented Aug 24, 2023 •

edited

Loading

jagabomb commented Aug 24, 2023

arbulu89 Aug 24, 2023

arbulu89 Aug 24, 2023

arbulu89 Aug 24, 2023

jagabomb left a comment

arbulu89 left a comment

arbulu89 Aug 24, 2023

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

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

Conversation

rtorrero commented Aug 23, 2023

Description

arbulu89 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arbulu89 left a comment

Choose a reason for hiding this comment

CDimonaco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arbulu89 left a comment

Choose a reason for hiding this comment

jagabomb commented Aug 24, 2023

CDimonaco commented Aug 24, 2023

rtorrero commented Aug 24, 2023 • edited Loading

jagabomb commented Aug 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jagabomb left a comment

Choose a reason for hiding this comment

arbulu89 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtorrero commented Aug 24, 2023 •

edited

Loading