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

fix: Workflows - Offline indicator is present below the last option instead of page bottom. #42019

Merged
merged 15 commits into from
May 31, 2024
Merged

fix: Workflows - Offline indicator is present below the last option instead of page bottom. #42019

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
422ba55
fix: Workflows - Offline indicator is present below the last option i…
Krishna2323 May 11, 2024
6ddaad4
minor update.
Krishna2323 May 11, 2024
47abac3
minor fix.
Krishna2323 May 12, 2024
ce82f83
update focus index when initialFocusedIndex changes.
Krishna2323 May 12, 2024
5ebd6d9
Merge branch 'Expensify:main' into krishna2323/issue/41326
Krishna2323 May 17, 2024
9350a31
remove redundant code.
Krishna2323 May 20, 2024
2cbeb45
rename footerComponent to footerContent.
Krishna2323 May 20, 2024
df2b6b7
fix typo.
Krishna2323 May 22, 2024
0be8338
Merge branch 'main' of https://github.com/Krishna2323/App into krishn…
Krishna2323 May 24, 2024
b565f2c
Merge branch 'main' of https://github.com/Krishna2323/App into krishn…
Krishna2323 May 28, 2024
8106011
Merge branch 'main' of https://github.com/Krishna2323/App into krishn…
Krishna2323 May 28, 2024
f5e5f49
Merge branch 'main' into krishna2323/issue/41326
Krishna2323 May 29, 2024
9e8e01b
Merge branch 'main' into krishna2323/issue/41326
Krishna2323 May 30, 2024
0f58c9a
remove extra line break.
Krishna2323 May 30, 2024
cf0be8a
Merge branch 'Expensify:main' into krishna2323/issue/41326
Krishna2323 May 30, 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
39 changes: 21 additions & 18 deletions src/components/SelectionList/BaseSelectionList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,24 +344,27 @@ function BaseSelectionList<TItem extends ListItem>(
const showTooltip = shouldShowTooltips && normalizedIndex < 10;

return (
<ListItem
item={item}
isFocused={isItemFocused}
isDisabled={isDisabled}
showTooltip={showTooltip}
canSelectMultiple={canSelectMultiple}
onSelectRow={() => selectRow(item)}
onCheckboxPress={onCheckboxPress ? () => onCheckboxPress?.(item) : undefined}
onDismissError={() => onDismissError?.(item)}
shouldPreventDefaultFocusOnSelectRow={shouldPreventDefaultFocusOnSelectRow}
// We're already handling the Enter key press in the useKeyboardShortcut hook, so we don't want the list item to submit the form
shouldPreventEnterKeySubmit
rightHandSideComponent={rightHandSideComponent}
keyForList={item.keyForList ?? ''}
isMultilineSupported={isRowMultilineSupported}
onFocus={() => setFocusedIndex(normalizedIndex)}
shouldSyncFocus={!isTextInputFocusedRef.current}
/>
<>
<ListItem
item={item}
isFocused={isItemFocused}
isDisabled={isDisabled}
showTooltip={showTooltip}
canSelectMultiple={canSelectMultiple}
onSelectRow={() => selectRow(item)}
onCheckboxPress={onCheckboxPress ? () => onCheckboxPress?.(item) : undefined}
onDismissError={() => onDismissError?.(item)}
shouldPreventDefaultFocusOnSelectRow={shouldPreventDefaultFocusOnSelectRow}
// We're already handling the Enter key press in the useKeyboardShortcut hook, so we don't want the list item to submit the form
shouldPreventEnterKeySubmit
rightHandSideComponent={rightHandSideComponent}
keyForList={item.keyForList ?? ''}
isMultilineSupported={isRowMultilineSupported}
onFocus={() => setFocusedIndex(normalizedIndex)}
shouldSyncFocus={!isTextInputFocusedRef.current}
/>
{item.footerComponent && item.footerComponent}
</>
);
};

Expand Down
3 changes: 3 additions & 0 deletions src/components/SelectionList/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,9 @@ type ListItem = {

/** Whether the brick road indicator should be shown */
brickRoadIndicator?: BrickRoad | '' | null;

/** Element to render below the ListItem */
footerComponent?: ReactNode;
};

type TransactionListItemType = ListItem & {
Expand Down
4 changes: 4 additions & 0 deletions src/hooks/useArrowKeyFocusManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,10 @@ export default function useArrowKeyFocusManager({
// eslint-disable-next-line react-hooks/exhaustive-deps
useEffect(() => onFocusedIndexChange(focusedIndex), [focusedIndex]);

useEffect(() => {
setFocusedIndex(initialFocusedIndex);
}, [initialFocusedIndex]);

Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need this change @Krishna2323?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was a focus issue with the newly selected option. Although I don't completely remember the exact bug. I removed this code as I can't reproduce any bug like that.

const arrowUpCallback = useCallback(() => {
if (maxIndex < 0 || !isFocused) {
return;
Expand Down
44 changes: 19 additions & 25 deletions src/pages/workspace/workflows/WorkspaceAutoReportingFrequencyPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import type {StackScreenProps} from '@react-navigation/stack';
import React, {useState} from 'react';
import {FlatList} from 'react-native-gesture-handler';
import type {ValueOf} from 'type-fest';
import FullPageNotFoundView from '@components/BlockingViews/FullPageNotFoundView';
import HeaderWithBackButton from '@components/HeaderWithBackButton';
import MenuItem from '@components/MenuItem';
import OfflineWithFeedback from '@components/OfflineWithFeedback';
import ScreenWrapper from '@components/ScreenWrapper';
import SelectionList from '@components/SelectionList';
import RadioListItem from '@components/SelectionList/RadioListItem';
import useLocalize from '@hooks/useLocalize';
import useThemeStyles from '@hooks/useThemeStyles';
Expand All @@ -33,6 +33,7 @@ type WorkspaceAutoReportingFrequencyPageItem = {
text: string;
keyForList: string;
isSelected: boolean;
footerComponent?: React.ReactNode | null;
};

type AutoReportingFrequencyDisplayNames = Record<AutoReportingFrequencyKey, string>;
Expand All @@ -51,16 +52,6 @@ function WorkspaceAutoReportingFrequencyPage({policy, route}: WorkspaceAutoRepor
const styles = useThemeStyles();
const [isMonthlyFrequency, setIsMonthlyFrequency] = useState(policy?.autoReportingFrequency === CONST.POLICY.AUTO_REPORTING_FREQUENCIES.MONTHLY);

const autoReportingFrequencyItems: WorkspaceAutoReportingFrequencyPageItem[] = Object.keys(getAutoReportingFrequencyDisplayNames(preferredLocale)).map((frequencyKey) => {
const isSelected = policy?.autoReportingFrequency === frequencyKey;

return {
text: getAutoReportingFrequencyDisplayNames(preferredLocale)[frequencyKey as AutoReportingFrequencyKey] || '',
keyForList: frequencyKey,
isSelected,
};
});

const onSelectAutoReportingFrequency = (item: WorkspaceAutoReportingFrequencyPageItem) => {
Policy.setWorkspaceAutoReportingFrequency(policy?.id ?? '', item.keyForList as AutoReportingFrequencyKey);

Expand Down Expand Up @@ -106,16 +97,16 @@ function WorkspaceAutoReportingFrequencyPage({policy, route}: WorkspaceAutoRepor
</OfflineWithFeedback>
);

const renderItem = ({item}: {item: WorkspaceAutoReportingFrequencyPageItem}) => (
<>
<RadioListItem
item={item}
onSelectRow={() => onSelectAutoReportingFrequency(item)}
showTooltip={false}
/>
{isMonthlyFrequency && item.keyForList === CONST.POLICY.AUTO_REPORTING_FREQUENCIES.MONTHLY ? monthlyFrequencyDetails() : null}
</>
);
const autoReportingFrequencyItems: WorkspaceAutoReportingFrequencyPageItem[] = Object.keys(getAutoReportingFrequencyDisplayNames(preferredLocale)).map((frequencyKey) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

we can put it into a memo to memorize this output

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 don't think this is necessary, but if we decide to use memo, we would also need to wrap monthlyFrequencyDetails and getDescriptionText in useCallback.

Copy link
Contributor

Choose a reason for hiding this comment

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

uhm, let's keep it as it's

const isSelected = policy?.autoReportingFrequency === frequencyKey;

return {
text: getAutoReportingFrequencyDisplayNames(preferredLocale)[frequencyKey as AutoReportingFrequencyKey] || '',
keyForList: frequencyKey,
isSelected,
footerComponent: isMonthlyFrequency && frequencyKey === CONST.POLICY.AUTO_REPORTING_FREQUENCIES.MONTHLY ? monthlyFrequencyDetails() : null,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
footerComponent: isMonthlyFrequency && frequencyKey === CONST.POLICY.AUTO_REPORTING_FREQUENCIES.MONTHLY ? monthlyFrequencyDetails() : null,
FooterComponent: isMonthlyFrequency && frequencyKey === CONST.POLICY.AUTO_REPORTING_FREQUENCIES.MONTHLY ? monthlyFrequencyDetails() : null,

If it's a component we should name it capitalize

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, we use camelCase for components as well, and instead of component we use content, so I replaced footerComponent with footerContent.

Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't show footerContent when I select "Monthly" option. Could you take a look?

Screen.Recording.2024-05-21.at.22.23.44.mov

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, maybe you don't have access, I just removed that check for testing.

App/src/pages/workspace/workflows/WorkspaceWorkflowsPage.tsx

Line 53 in e8ae3c5

const canUseDelayedSubmission = Permissions.canUseWorkflowsDelayedSubmission(betas);

Copy link
Contributor

Choose a reason for hiding this comment

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

No I mean the extra option "Date of month" when user selects "Monthly" option. Can you retest it and record your screen if you can see that "Date of month" option when you select "Monthly" option? Thanks

Screenshot 2024-05-22 at 21 02 41

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, sorry there was a typo, I forgot to update footerComponent to footerContent in one place. Fixed now.

};
});

return (
<AccessOrNotFoundWrapper
Expand All @@ -140,11 +131,14 @@ function WorkspaceAutoReportingFrequencyPage({policy, route}: WorkspaceAutoRepor
pendingAction={policy?.pendingFields?.autoReportingFrequency}
errors={ErrorUtils.getLatestErrorField(policy ?? {}, CONST.POLICY.COLLECTION_KEYS.AUTOREPORTING_FREQUENCY)}
onClose={() => Policy.clearPolicyErrorField(policy?.id ?? '', CONST.POLICY.COLLECTION_KEYS.AUTOREPORTING_FREQUENCY)}
style={styles.flex1}
contentContainerStyle={styles.flex1}
>
<FlatList
data={autoReportingFrequencyItems}
renderItem={renderItem}
keyExtractor={(item: WorkspaceAutoReportingFrequencyPageItem) => item.text}
<SelectionList
Copy link
Contributor

Choose a reason for hiding this comment

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

Coming from this #47574
Since we don't pass shouldUpdateFocusedIndex we don't see selected background after selecting Submission frequency as monthly

ListItem={RadioListItem}
sections={[{data: autoReportingFrequencyItems}]}
onSelectRow={onSelectAutoReportingFrequency}
initiallyFocusedOptionKey={policy?.autoReportingFrequency}
/>
</OfflineWithFeedback>
</FullPageNotFoundView>
Expand Down
Loading