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

likhith/migrate dialog component to tsx #21

Original file line number Diff line number Diff line change
@@ -1,26 +1,47 @@
import React from 'react';
import classNames from 'classnames';
import PropTypes from 'prop-types';
import ReactDOM from 'react-dom';
import { CSSTransition } from 'react-transition-group';
import Icon from '../icon/icon';
import Button from '../button/button';
import Text from '../text';
import { useOnClickOutside } from '../../hooks';

type TDialog = {
cancel_button_text: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldnt cancel_button_text an optional param?

className?: string;
confirm_button_text?: string;
dismissable?: boolean;
disableApp?: () => void;
enableApp?: () => void;
has_close_icon?: boolean;
is_closed_on_cancel?: boolean;
is_closed_on_confirm?: boolean;
is_content_centered?: boolean;
is_loading?: boolean;
is_mobile_full_width?: boolean;
is_visible?: boolean;
onCancel?: () => void;
onClose?: () => void;
onConfirm: () => void;
onEscapeButtonCancel?: () => void;
portal_element_id?: string;
title?: string;
};

const Dialog = ({
disableApp,
dismissable,
enableApp,
is_closed_on_cancel,
is_closed_on_confirm,
is_closed_on_cancel = true,
is_closed_on_confirm = true,
is_visible,
onCancel,
onClose,
onConfirm,
onEscapeButtonCancel,
...other_props
}) => {
}: React.PropsWithChildren<TDialog>) => {
const {
cancel_button_text,
className,
Expand All @@ -34,7 +55,7 @@ const Dialog = ({
has_close_icon,
} = other_props;

const wrapper_ref = React.useRef();
const wrapper_ref = React.useRef() as React.MutableRefObject<HTMLInputElement>;

Choose a reason for hiding this comment

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

Suggested change
const wrapper_ref = React.useRef() as React.MutableRefObject<HTMLInputElement>;
const wrapper_ref = React.useRef<HTMLInputElement>


React.useEffect(() => {
if (is_visible && !!disableApp) {
Expand All @@ -43,7 +64,7 @@ const Dialog = ({
}, [is_visible, disableApp]);

React.useEffect(() => {
const close = e => {
const close = (e: { key: string }) => {
if (e.key === 'Escape') {
onEscapeButtonCancel?.();
}
Expand All @@ -68,9 +89,9 @@ const Dialog = ({
};

const handleClose = () => {
if (onClose) {
if (typeof onClose === 'function') {

Choose a reason for hiding this comment

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

why just not to have here onClose?.() and onCancel?.() instead of these if's ...? please consider it :)

Copy link
Author

@likhith-deriv likhith-deriv Oct 31, 2022

Choose a reason for hiding this comment

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

In this function it is better to check as there can be 3 functions used - onClose, handleCancel, handleConfirm.
But ur suggestion I will use in handleCancel

Choose a reason for hiding this comment

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

I believe there is no sense to check if onClose or onCancel are functions, because you set them as a functions in types. so if this function is not passes via props, this param is undefined.
so you can check this simply if (onClose) ....
how do you think?

onClose();
} else if (onCancel) {
} else if (typeof onCancel === 'function') {
handleCancel();
} else {
handleConfirm();
Expand All @@ -84,8 +105,10 @@ const Dialog = ({
const content_classes = classNames('dc-dialog__content', {
'dc-dialog__content--centered': is_content_centered,
});

const is_text = typeof children === 'string' || typeof children?.props?.i18n_default_text === 'string';
//
const is_text =
typeof children === 'string' ||
(React.isValidElement(children) && typeof children?.props?.i18n_default_text === 'string');
const dialog = (
<CSSTransition
appear
Expand Down Expand Up @@ -164,34 +187,10 @@ const Dialog = ({
);

if (portal_element_id) {
return ReactDOM.createPortal(dialog, document.getElementById(portal_element_id));
return ReactDOM.createPortal(dialog, document.getElementById(portal_element_id)!); // non-null assertion
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we check if document.getElementById(portal_element_id) exists in the if condition instead of disabling non - null assertion . Something like-

if (portal_element_id && document.getElementById(portal_element_id)!==null){
..
 }

}

return dialog;
};

Dialog.defaultProps = {
is_closed_on_cancel: true,
is_closed_on_confirm: true,
};

Dialog.propTypes = {
cancel_button_text: PropTypes.string,
confirm_button_text: PropTypes.string,
disableApp: PropTypes.func,
enableApp: PropTypes.func,
has_close_icon: PropTypes.bool,
is_closed_on_cancel: PropTypes.bool,
is_closed_on_confirm: PropTypes.bool,
is_content_centered: PropTypes.bool,
is_loading: PropTypes.bool,
is_mobile_full_width: PropTypes.bool,
is_visible: PropTypes.bool,
onCancel: PropTypes.func,
onEscapeButtonCancel: PropTypes.func,
onConfirm: PropTypes.func,
portal_element_id: PropTypes.string,
title: PropTypes.string,
};

export default Dialog;
2 changes: 1 addition & 1 deletion packages/components/src/components/dialog/index.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import Dialog from './dialog.jsx';
import Dialog from './dialog';
import './dialog.scss';

export default Dialog;