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

Make sure Dialogs do not render close button when onClose callback is not provided #1045

Merged
merged 1 commit into from
May 23, 2023
Merged

Make sure Dialogs do not render close button when onClose callback is not provided #1045

Show file tree
Hide file tree
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
11 changes: 5 additions & 6 deletions src/components/feedback/Dialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,6 @@ export type DialogProps = PresentationalProps &
ComponentProps &
Omit<PanelProps, 'fullWidthHeader'>;

const noop = () => {};

/**
* Show a dialog
*/
Expand All @@ -74,7 +72,7 @@ const Dialog = function Dialog({
// Forwarded to Panel
buttons,
icon,
onClose = noop,
onClose,
paddingSize = 'md',
scrollable = true,
title,
Expand All @@ -87,14 +85,15 @@ const Dialog = function Dialog({
);
const [transitionComponentVisible, setTransitionComponentVisible] =
useState(false);
const isClosableDialog = !!onClose;

const closeHandler = useCallback(() => {
if (TransitionComponent) {
// When a TransitionComponent is provided, the actual "onClose" will be
// called by that component, once the "out" transition has finished
setTransitionComponentVisible(false);
} else {
onClose();
onClose?.();
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically we know that onClose is present at this point, but TS wouldn't, I suppose. I'm fine leaving this as-is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's actually a good point 🤔

I kind-of want to give this a thought and see if I can find a way to make TS know this is actually set at this point.

}
}, [onClose, TransitionComponent]);

Expand Down Expand Up @@ -127,7 +126,7 @@ const Dialog = function Dialog({
if (direction === 'in') {
initializeDialog();
} else {
onClose();
onClose?.();
}
};

Expand Down Expand Up @@ -216,7 +215,7 @@ const Dialog = function Dialog({
buttons={buttons}
fullWidthHeader={true}
icon={icon}
onClose={closeHandler}
onClose={isClosableDialog ? closeHandler : undefined}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could just have done onClose ? closeHandler : undefined, but I feel starting with isClosableDialog explains better the "why".

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's fine.

paddingSize={paddingSize}
title={title}
scrollable={scrollable}
Expand Down
7 changes: 7 additions & 0 deletions src/components/feedback/test/Dialog-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,13 @@ describe('Dialog', () => {
assert.called(onClose);
});
});

context('when no onClose callback is provided', () => {
it('does not render a close button', () => {
const wrapper = mount(<Dialog title="My dialog" />);
assert.isFalse(wrapper.find('CancelIcon').exists());
});
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test fails in main.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice job, thanks!

});

describe('aria-describedby', () => {
Expand Down