Add prefer-at condition to eslint #49534
Conversation
.github/actions/javascript/proposalPoliceComment/proposalPoliceComment.ts
Outdated
Show resolved
Hide resolved
src/components/MenuItem.tsx
Outdated
| @@ -20,6 +20,7 @@ import CONST from '@src/CONST'; | |||
| import type {Icon as IconType} from '@src/types/onyx/OnyxCommon'; | |||
| import type {TooltipAnchorAlignment} from '@src/types/utils/AnchorAlignment'; | |||
| import type IconAsset from '@src/types/utils/IconAsset'; | |||
| import getDefaultIcon from '@src/utils/getDefaultIcon'; | |||
There was a problem hiding this comment.
| import getDefaultIcon from '@src/utils/getDefaultIcon'; | |
| import fallbackIcon from '@src/utils/getDefaultIcon'; |
| @@ -309,8 +309,12 @@ function MoneyRequestView({report, shouldShowAnimatedBackground, readonly = fals | |||
|
|
|||
| // Return violations if there are any | |||
| if (hasViolations(field, data, policyHasDependentTags, tagValue)) { | |||
| const violations = getViolationsForField(field, data, policyHasDependentTags, tagValue); | |||
| return ViolationsUtils.getViolationTranslation(violations[0], translate); | |||
| const violations = getViolationsForField(field, data, policyHasDependentTags, tagValue) as OnyxTypes.TransactionViolation[]; | |||
There was a problem hiding this comment.
Please, make sure getViolationsForField returns OnyxTypes.TransactionViolation[] same way as it was before.
| const violations = getViolationsForField(field, data, policyHasDependentTags, tagValue) as OnyxTypes.TransactionViolation[]; | |
| const violations = getViolationsForField(field, data, policyHasDependentTags, tagValue); |
| @@ -651,7 +655,7 @@ function MoneyRequestView({report, shouldShowAnimatedBackground, readonly = fals | |||
| <Text color={!transactionBillable ? theme.textSupporting : undefined}>{translate('common.billable')}</Text> | |||
| {!!getErrorForField('billable') && ( | |||
| <ViolationMessages | |||
| violations={getViolationsForField('billable')} | |||
| violations={getViolationsForField('billable') as OnyxTypes.TransactionViolation[]} | |||
There was a problem hiding this comment.
Same
| violations={getViolationsForField('billable') as OnyxTypes.TransactionViolation[]} | |
| violations={getViolationsForField('billable')} |
| if (policyHasDependentTags && currentViolations[0]?.name === CONST.VIOLATIONS.MISSING_TAG && data?.tagListName) { | ||
| if (policyHasDependentTags && currentViolations.at(0)?.name === CONST.VIOLATIONS.MISSING_TAG && data?.tagListName) { | ||
| return [ | ||
| { | ||
| ...currentViolations[0], | ||
| ...currentViolations.at(0), | ||
| data: { | ||
| ...currentViolations[0].data, | ||
| ...currentViolations.at(0)?.data, | ||
| tagName: data?.tagListName, | ||
| }, | ||
| }, | ||
| ]; | ||
| } |
There was a problem hiding this comment.
It looks like due to these updates the return type of getViolationsForField has changed. To fix it you can put
currentViolations.at(0) into const and reuse it:
const firstViolation = currentViolations.at(0);
...
if (policyHasDependentTags && firstViolation?.name === CONST.VIOLATIONS.MISSING_TAG && data?.tagListName) {
return [
{
...firstViolation,
data: {
...firstViolation.data,
tagName: data?.tagListName,
},
},
];
}
| @@ -242,7 +242,7 @@ function BaseSelectionList<TItem extends ListItem>( | |||
| */ | |||
| const scrollToIndex = useCallback( | |||
| (index: number, animated = true) => { | |||
| const item = flattenedSections.allOptions[index]; | |||
| const item = flattenedSections.allOptions.at(index) ?? 0; | |||
There was a problem hiding this comment.
Please make sure indexes in this file won't be negative since it can affect the logic then.
| @@ -847,7 +851,7 @@ function getFirstVisibleReportActionID(sortedReportActions: ReportAction[] = [], | |||
| return ''; | |||
| } | |||
| const sortedFilterReportActions = sortedReportActions.filter((action) => !isDeletedAction(action) || (action?.childVisibleActionCount ?? 0) > 0 || isOffline); | |||
| return sortedFilterReportActions.length > 1 ? sortedFilterReportActions[sortedFilterReportActions.length - 2].reportActionID : ''; | |||
| return sortedFilterReportActions.length > 1 ? sortedFilterReportActions.at(sortedFilterReportActions.length - 2)?.reportActionID ?? '-1' : ''; | |||
There was a problem hiding this comment.
Make sure that sortedFilterReportActions.length - 2 won't be negative, since the update can affect the logic then.
| @@ -1437,7 +1441,7 @@ function isCurrentActionUnread(report: OnyxEntry<Report>, reportAction: ReportAc | |||
| if (currentActionIndex === -1) { | |||
| return false; | |||
| } | |||
| const prevReportAction = sortedReportActions[currentActionIndex - 1]; | |||
| const prevReportAction = sortedReportActions.at(currentActionIndex - 1); | |||
There was a problem hiding this comment.
Make sure that currentActionIndex - 1 won't be negative, since it can affect the logic then.
| @@ -139,7 +155,7 @@ function SearchTypeMenuNarrow({typeMenuItems, activeItemIndex, queryJSON, title, | |||
| <View style={[styles.pb4, styles.flexRow, styles.alignItemsCenter, styles.justifyContentBetween, styles.ph5, styles.gap2]}> | |||
| <PressableWithFeedback | |||
| accessible | |||
| accessibilityLabel={popoverMenuItems[activeItemIndex]?.text ?? ''} | |||
| accessibilityLabel={popoverMenuItems.at(activeItemIndex)?.text ?? ''} | |||
There was a problem hiding this comment.
Please make sure activeItemIndex won't be negative since it can affect the logic then.
| @@ -357,7 +360,7 @@ function SuggestionMention( | |||
| const leftString = newValue.substring(afterLastBreakLineIndex, selectionEnd); | |||
| const words = leftString.split(CONST.REGEX.SPACE_OR_EMOJI); | |||
| const lastWord: string = words.at(-1) ?? ''; | |||
| const secondToLastWord = words[words.length - 3]; | |||
| const secondToLastWord = words.at(words.length - 3); | |||
There was a problem hiding this comment.
Please, make sure it doesn't break anything, cause .at() accepts negative integers while [] not. See #49534 (comment)
| assignedCardsGrouped[domainGroupIndex].errors = {...assignedCardsGrouped[domainGroupIndex].errors, ...card.errors}; | ||
| if (card.fraud === CONST.EXPENSIFY_CARD.FRAUD_TYPES.DOMAIN || card.fraud === CONST.EXPENSIFY_CARD.FRAUD_TYPES.INDIVIDUAL) { | ||
| assignedCardsGrouped[domainGroupIndex].brickRoadIndicator = CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR; | ||
| const assignedCardsGroupedItem = assignedCardsGrouped.at(domainGroupIndex); |
There was a problem hiding this comment.
Please, make sure it doesn't break anything, cause .at() accepts negative integers while [] not. See #49534 (comment)
| <OfflineWithFeedback | ||
| errors={policy?.errorFields?.requiresTag} | ||
| pendingAction={policy?.pendingFields?.requiresTag} | ||
| errorRowStyles={styles.mh5} | ||
| > | ||
| <View style={[styles.flexRow, styles.mh5, styles.mv4, styles.alignItemsCenter, styles.justifyContentBetween]}> | ||
| <Text style={[styles.textNormal]}>{translate('workspace.tags.requiresTag')}</Text> | ||
| <Switch | ||
| isOn={policy?.requiresTag ?? false} | ||
| accessibilityLabel={translate('workspace.tags.requiresTag')} | ||
| onToggle={updateWorkspaceRequiresTag} | ||
| disabled={!policy?.areTagsEnabled || !hasEnabledOptions} | ||
| /> | ||
| </View> | ||
| </OfflineWithFeedback> | ||
| {canUseWorkspaceRules && policy?.areRulesEnabled && ( | ||
| <OfflineWithFeedback pendingAction={billableExpensesPending(policy)}> | ||
| <View style={[styles.flexRow, styles.mh5, styles.mv4, styles.alignItemsCenter, styles.justifyContentBetween]}> | ||
| <Text style={[styles.textNormal]}>{translate('workspace.tags.trackBillable')}</Text> | ||
| <Switch | ||
| isOn={!(policy?.disabledFields?.defaultBillable ?? false)} | ||
| accessibilityLabel={translate('workspace.tags.trackBillable')} | ||
| onToggle={() => toggleBillableExpenses(policy)} | ||
| /> | ||
| </View> | ||
| </OfflineWithFeedback> | ||
| )} | ||
| <OfflineWithFeedback | ||
| errors={policy?.errorFields?.requiresTag} | ||
| pendingAction={policy?.pendingFields?.requiresTag} | ||
| errorRowStyles={styles.mh5} | ||
| > | ||
| <View style={[styles.flexRow, styles.mh5, styles.mv4, styles.alignItemsCenter, styles.justifyContentBetween]}> | ||
| <Text style={[styles.textNormal]}>{translate('workspace.tags.requiresTag')}</Text> | ||
| <Switch | ||
| isOn={policy?.requiresTag ?? false} | ||
| accessibilityLabel={translate('workspace.tags.requiresTag')} | ||
| onToggle={updateWorkspaceRequiresTag} | ||
| disabled={!policy?.areTagsEnabled || !hasEnabledOptions} | ||
| /> | ||
| </View> | ||
| </OfflineWithFeedback> | ||
| {canUseWorkspaceRules && policy?.areRulesEnabled && ( | ||
| <OfflineWithFeedback pendingAction={billableExpensesPending(policy)}> | ||
| <View style={[styles.flexRow, styles.mh5, styles.mv4, styles.alignItemsCenter, styles.justifyContentBetween]}> | ||
| <Text style={[styles.textNormal]}>{translate('workspace.tags.trackBillable')}</Text> | ||
| <Switch | ||
| isOn={!(policy?.disabledFields?.defaultBillable ?? false)} | ||
| accessibilityLabel={translate('workspace.tags.trackBillable')} | ||
| onToggle={() => toggleBillableExpenses(policy)} | ||
| /> | ||
| </View> | ||
| </OfflineWithFeedback> | ||
| )} |
There was a problem hiding this comment.
Should it be in this PR?
| @@ -1,4 +1,6 @@ | |||
| /* eslint-disable @typescript-eslint/naming-convention */ | |||
|
|
|||
| /* eslint-disable rulesdir/prefer-at */ | |||
There was a problem hiding this comment.
Why we disable this rule here?
| measurePerformance(<BaseOptionsListWrapper />); | ||
| measureRenders(<BaseOptionsListWrapper />); |
There was a problem hiding this comment.
Why was it changed in this PR? (relates to all these changes in this PR)
There was a problem hiding this comment.
Ah then I just wanted to fix to check and it seemed like a quick change but I can revert it
| @@ -1,3 +1,4 @@ | |||
| /* eslint-disable rulesdir/prefer-at */ | |||
There was a problem hiding this comment.
Why we disable this rule?
| @@ -1,3 +1,4 @@ | |||
| /* eslint-disable rulesdir/prefer-at */ | |||
There was a problem hiding this comment.
Same, why we disable it?
|
|
||
| /* eslint-disable rulesdir/prefer-at */ |
There was a problem hiding this comment.
Related to this comment: #49534 (comment) and, more general - default icon and avatars, a few cents from me.
First of all I agree with Błażej to improve naming. This should be correctly named fallbackIcon and not a defaultIcon.
Second of all we cannot treat this:
source: FallbackAvatar,
type: CONST.ICON_TYPE_AVATAR,
as universal fallback icon everywhere, because this is specifically a fallback Avatar, not a "default" icon or avatar.
This means that if something is to display a different TYPE of icon, and we fallback to FallbackAvatar then it might be incorrect.
For the record this is fallback avatar:
As you can see displaying this image in place of for example something report or workspace related might be problematic.
Because of ☝️ my suggestion would be:
- do not add default value for
mainAvatarinsideSubscriptAvatar - do not create a separate file with fallbackIcon
Even though defaulting mainAvatar sounds like a good idea, it will hide a lot of wrong usages of SubscriptAvatar.
I think we should go case by case and verify where the fallback value is even needed. I did a quick check and found several usages where we don't have to fallback, but instead we should refactor code:
- in
OptionRowwe have this check:{!!option.icons?.length && ...so we don't have to even display Avatar if there is no icons - we should clean this code and not fallback - in
MenuItemwe havefloatRightAvatars?.length > 0- same as above
Everywhere else define fallbackIcon locally and use it for the specific files.
Details
Fixed Issues
$ #43055
PROPOSAL:
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop