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

fix: Room - Side panel does not close when clicking outside of panel. #38015

Merged
merged 4 commits into from
Mar 19, 2024
Merged

fix: Room - Side panel does not close when clicking outside of panel. #38015

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
904316b
fix: Room - Side panel does not close when clicking outside of panel.
Krishna2323 Mar 9, 2024
f080b78
Merge branch 'Expensify:main' into krishna2323/re/issue/34394
Krishna2323 Mar 15, 2024
cc63c22
add suggested changes.
Krishna2323 Mar 15, 2024
a5af583
Merge branch 'main' into krishna2323/re/issue/34394
Krishna2323 Mar 18, 2024
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
4 changes: 4 additions & 0 deletions src/components/Modal/BaseModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ function BaseModal(
avoidKeyboard = false,
children,
shouldUseCustomBackdrop = false,
onBackdropPress,
}: BaseModalProps,
ref: React.ForwardedRef<View>,
) {
Expand Down Expand Up @@ -117,6 +118,9 @@ function BaseModal(
return;
}

if (onBackdropPress) {
onBackdropPress();
}
onClose();
Copy link
Contributor

@hoangzinh hoangzinh Mar 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@Krishna2323 should it be?

if (onBackdropPress) {
  onBackdropPress();
} else {
  onClose();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, onClose must be called but onBackdropPress is optional.

Copy link
Contributor

Choose a reason for hiding this comment

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

then it would be weird @Krishna2323. Let's say

<Modal
  onClose={do_on_close}
  onBackdropPress={do_on_bd_press)
/>

If you haven't read the implementation details of this Modal component, we might thought when backdrop press, it's only do_on_bd_press. We aren't aware it would call onClose as well.

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 we should give decisions to users, where they can decide if they want to call do_on_close when bd press as well

<Modal
  onClose={do_on_close}
  onBackdropPress={do_on_bd_press && do_on_close)
/>

or

<Modal
  onClose={do_on_close}
  onBackdropPress={do_on_close && do_on_bd_press)
/>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it looks confusing, let me check again.

Copy link
Contributor

Choose a reason for hiding this comment

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

cc @Krishna2323 just in case you missed my comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hoangzinh, apologies for the delay. I believe we can proceed as you suggested without causing any regressions at this time. However, my main concern is regarding the difference between onClose and onBackdropPress callbacks.

In Scenario 1: If we update states when the onClose callback is triggered and we have specific actions for when the user clicks on the backdrop, solely calling onBackdropPress might not achieve the intended functionality.

In Scenario 2: If we update states when either onClose or onBackdropPress is called, it might be better to only utilize one of these to avoid potential regressions due to redundant function calls.

At the moment, I'm inclined towards only calling one of these callbacks as you suggested, but I'd like to confirm our decision once more. Please let me know your thoughts on how we should proceed.

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:

  • In scenario 1: we can call onClose inside onBackdropPress

Let's simplify it, just go with only calling one of these callbacks, then we can update later on our way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hoangzinh, updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hoangzinh friendly bump

};

Expand Down
3 changes: 3 additions & 0 deletions src/components/Modal/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ type BaseModalProps = Partial<ModalProps> & {
/** Callback method fired when the user requests to close the modal */
onClose: () => void;

/** Function to call when the user presses on the modal backdrop */
onBackdropPress?: () => void;

/** State that determines whether to display the modal or not */
isVisible: boolean;

Expand Down
3 changes: 2 additions & 1 deletion src/components/ValuePicker/ValueSelectorModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import useThemeStyles from '@hooks/useThemeStyles';
import CONST from '@src/CONST';
import type {ValueSelectorModalProps} from './types';

function ValueSelectorModal({items = [], selectedItem, label = '', isVisible, onClose, onItemSelected, shouldShowTooltips = true}: ValueSelectorModalProps) {
function ValueSelectorModal({items = [], selectedItem, label = '', isVisible, onClose, onItemSelected, shouldShowTooltips = true, onBackdropPress}: ValueSelectorModalProps) {
const styles = useThemeStyles();

const sections = useMemo(
Expand All @@ -24,6 +24,7 @@ function ValueSelectorModal({items = [], selectedItem, label = '', isVisible, on
onModalHide={onClose}
hideModalContentWhileAnimating
useNativeDriver
onBackdropPress={onBackdropPress}
>
<ScreenWrapper
style={styles.pb0}
Expand Down
2 changes: 2 additions & 0 deletions src/components/ValuePicker/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import FormHelpMessage from '@components/FormHelpMessage';
import MenuItemWithTopDescription from '@components/MenuItemWithTopDescription';
import useStyleUtils from '@hooks/useStyleUtils';
import useThemeStyles from '@hooks/useThemeStyles';
import Navigation from '@libs/Navigation/Navigation';
import variables from '@styles/variables';
import type {ValuePickerItem, ValuePickerProps} from './types';
import ValueSelectorModal from './ValueSelectorModal';
Expand Down Expand Up @@ -55,6 +56,7 @@ function ValuePicker({value, label, items, placeholder = '', errorText = '', onI
onClose={hidePickerModal}
onItemSelected={updateInput}
shouldShowTooltips={shouldShowTooltips}
onBackdropPress={Navigation.dismissModal}
/>
</View>
);
Expand Down
3 changes: 3 additions & 0 deletions src/components/ValuePicker/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ type ValueSelectorModalProps = {
/** Function to call when the user closes the modal */
onClose?: () => void;

/** Function to call when the user presses on the modal backdrop */
onBackdropPress?: () => void;

/** Whether to show the toolip text */
shouldShowTooltips?: boolean;
};
Expand Down
Loading