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

Jump to bottom

chore: add onDismiss handler for TaxPicker #43521

Merged
merged 5 commits into from
Jun 17, 2024
Merged

chore: add onDismiss handler for TaxPicker #43521

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
82c6dd3
chore: add onDismiss handler for TaxPicker
dominictb Jun 12, 2024
443bc2b
fix: add dismiss func in IOURequestStepTaxRate
dominictb Jun 12, 2024
5292617
fix: prettier
dominictb Jun 12, 2024
c62c3bc
Merge remote-tracking branch 'origin/main' into fix/42854-tax-picker
dominictb Jun 14, 2024
7c9396e
fix: rename functions
dominictb Jun 14, 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
19 changes: 16 additions & 3 deletions src/components/TaxPicker.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, {useMemo, useState} from 'react';
import React, {useCallback, useMemo, useState} from 'react';
import {withOnyx} from 'react-native-onyx';
import type {OnyxEntry} from 'react-native-onyx';
import type {EdgeInsets} from 'react-native-safe-area-context';
Expand Down Expand Up @@ -54,9 +54,11 @@ type TaxPickerProps = TaxPickerOnyxProps & {

/** The type of IOU */
iouType?: ValueOf<typeof CONST.IOU.TYPE>;

onDismiss: () => void;
};
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 goBack or navigateBack name will be more correct and meaningful 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

onDismiss is fine. It matches with onSubmit handler name

Copy link
Contributor

Choose a reason for hiding this comment

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

@dominictb Yeah, but in this case, we execute the goBack action not dismiss action. As I understand about our codebase, dismiss action only be triggered when we want to turn off the modal or keyboard.

Copy link
Contributor Author

@dominictb dominictb Jun 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think it makes sense. Inside the TaxPicker, onDismiss is an generic event. It shouldn't be specific to any action like goBack or closeModal.

Copy link
Contributor

Choose a reason for hiding this comment

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

My concern about the dismiss word 😄. It doesn't match with the implementation of this function, this function executes goBack action

Copy link
Contributor Author

@dominictb dominictb Jun 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

goBack is too specific. What if the TaxPicker is inside a modal and you want to dismiss it? I think, dismiss is a generic word referencing a generic action/event in this case, i.e Do nothing case.


function TaxPicker({selectedTaxRate = '', policy, transaction, insets, onSubmit, action, splitDraftTransaction, iouType}: TaxPickerProps) {
function TaxPicker({selectedTaxRate = '', policy, transaction, insets, onSubmit, action, splitDraftTransaction, iouType, onDismiss}: TaxPickerProps) {
const StyleUtils = useStyleUtils();
const {translate} = useLocalize();
const [searchValue, setSearchValue] = useState('');
Expand Down Expand Up @@ -94,14 +96,25 @@ function TaxPicker({selectedTaxRate = '', policy, transaction, insets, onSubmit,

const selectedOptionKey = useMemo(() => sections?.[0]?.data?.find((taxRate) => taxRate.searchText === selectedTaxRate)?.keyForList, [sections, selectedTaxRate]);

const handleSelectRow = useCallback(
(newSelectedOption: OptionsListUtils.TaxRatesOption) => {
if (selectedOptionKey === newSelectedOption.keyForList) {
onDismiss();
return;
}
onSubmit(newSelectedOption);
},
[onSubmit, onDismiss, selectedOptionKey],
);

return (
<SelectionList
sections={sections}
headerMessage={headerMessage}
textInputValue={searchValue}
textInputLabel={shouldShowTextInput ? translate('common.search') : undefined}
onChangeText={setSearchValue}
onSelectRow={onSubmit}
onSelectRow={handleSelectRow}
ListItem={RadioListItem}
initiallyFocusedOptionKey={selectedOptionKey ?? undefined}
isRowMultilineSupported
Expand Down
5 changes: 1 addition & 4 deletions src/pages/iou/request/step/IOURequestStepTaxRatePage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,10 +85,6 @@ function IOURequestStepTaxRatePage({

if (isEditing) {
const newTaxCode = taxes.code;
if (newTaxCode === TransactionUtils.getTaxCode(currentTransaction)) {
navigateBack();
return;
}
IOU.updateMoneyRequestTaxRate({
transactionID: currentTransaction?.transactionID ?? '-1',
optimisticReportActionID: report?.reportID ?? '-1',
Expand Down Expand Up @@ -127,6 +123,7 @@ function IOURequestStepTaxRatePage({
onSubmit={updateTaxRates}
action={action}
iouType={iouType}
onDismiss={navigateBack}
/>
</StepScreenWrapper>
);
Expand Down
5 changes: 5 additions & 0 deletions src/pages/workspace/taxes/WorkspaceTaxesSettingsForeignCurrency.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,10 @@ function WorkspaceTaxesSettingsForeignCurrency({
Navigation.goBack(ROUTES.WORKSPACE_TAXES_SETTINGS.getRoute(policyID));
};

const dismiss = () => {
Navigation.goBack(ROUTES.WORKSPACE_TAXES_SETTINGS.getRoute(policyID));
};

return (
<AccessOrNotFoundWrapper
accessVariants={[CONST.POLICY.ACCESS_VARIANTS.ADMIN, CONST.POLICY.ACCESS_VARIANTS.PAID]}
Expand All @@ -60,6 +64,7 @@ function WorkspaceTaxesSettingsForeignCurrency({
policyID={policyID}
insets={insets}
onSubmit={submit}
onDismiss={dismiss}
/>
</View>
</>
Expand Down
5 changes: 5 additions & 0 deletions src/pages/workspace/taxes/WorkspaceTaxesSettingsWorkspaceCurrency.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,10 @@ function WorkspaceTaxesSettingsWorkspaceCurrency({
Navigation.goBack(ROUTES.WORKSPACE_TAXES_SETTINGS.getRoute(policyID));
};

const dismiss = () => {
Navigation.goBack(ROUTES.WORKSPACE_TAXES_SETTINGS.getRoute(policyID));
};

return (
<AccessOrNotFoundWrapper
accessVariants={[CONST.POLICY.ACCESS_VARIANTS.ADMIN, CONST.POLICY.ACCESS_VARIANTS.PAID]}
Expand All @@ -60,6 +64,7 @@ function WorkspaceTaxesSettingsWorkspaceCurrency({
policyID={policyID}
insets={insets}
onSubmit={submit}
onDismiss={dismiss}
/>
</View>
</>
Expand Down
Loading