Migrate Button to ButtonComposed (batch 5) search+reports - #97336
Migrate Button to ButtonComposed (batch 5) search+reports#97336mikolajpochec wants to merge 8 commits into
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
| isNested | ||
| medium | ||
| size={CONST.BUTTON_SIZE.MEDIUM} | ||
| innerStyles={[styles.ph3]} |
There was a problem hiding this comment.
AI:
The horizontal padding of this button changed with the migration.
Before: the legacy
mediumcontainer hadpaddingHorizontal: 16, andinnerStyles={[styles.ph3]}overrode it to 12, with the text flush against that padding - so the label sat 12px from the edge.After: the new
Buttonalready appliesph3for the medium size, so thisinnerStyles={[styles.ph3]}is a no-op, andButton.Textalways adds its ownph1. The label now sits 16px from the edge and the button is ~8px wider than before.To keep the old look 1:1:
Suggested change
innerStyles={[styles.ph3]} innerStyles={[styles.ph2]} (8px container + 4px text = 12px, same as before.)
Flagging this one specifically because "Show more" is on the "no reachable path" list in the PR description, so the difference was not caught on screen.
| type ActionableItemButtonsProps = { | ||
| items: ActionableItem[]; | ||
| layout?: 'horizontal' | 'vertical'; | ||
| shouldUseLocalization?: boolean; | ||
| primaryTextNumberOfLines?: number; | ||
| styles?: { | ||
| text?: StyleProp<TextStyle>; | ||
| button?: StyleProp<ViewStyle>; | ||
| }; | ||
| wrapperStyle?: StyleProp<ViewStyle>; | ||
| }; |
There was a problem hiding this comment.
I would change the names of the props to reflect what the new Button uses.
It would be a good idea to use TypeScript's Pick<> for this purpose.
| numberOfLines={props.primaryTextNumberOfLines} | ||
| style={props.styles?.text} | ||
| > | ||
| {props.shouldUseLocalization ? translate(item.text as TranslationPaths) : item.text} |
There was a problem hiding this comment.
NAB, follow-up idea (pre-existing API, not something this PR changed): the shouldUseLocalization flag + text doing double duty (translation key or literal text) could become two mutually exclusive per-item props:
type ActionableItem = {
isPrimary?: boolean;
key: string;
onPress: () => void;
} & ({translationKey: TranslationPaths; text?: never} | {text: string; translationKey?: never});and the render becomes item.translationKey ? translate(item.translationKey) : item.text with no cast.
There was a problem hiding this comment.
Great idea, applying this change.
Guccio163
left a comment
Guccio163
left a comment
There was a problem hiding this comment.
Generally good PR, left some minor comments 👀
| wrapperStyle?: StyleProp<ViewStyle>; | ||
| }; | ||
|
|
||
| function ActionableItemButtons(props: ActionableItemButtonsProps) { |
There was a problem hiding this comment.
I would consider migrating this whole component (ActionableItemButtons) as well - if you look at the usages they are pretty much hardcoded as well, conditionals-dependent. ActionableItemButtons basically passes props down (beside wrapping its children), so look like it could use the composition 🏗️ , WDYT @dariusz-biela ?
| large={largeButton} | ||
| pressOnEnter | ||
| variant={CONST.BUTTON_VARIANT.SUCCESS} | ||
| size={largeButton ? CONST.BUTTON_SIZE.LARGE : undefined} |
There was a problem hiding this comment.
I'd consider changing DateFilterContent's largeButton to optional size - it surely opens doors for passing the 'small' option, but shouldn't be a problem. If concerned, we can always use Exclude/Extract in the prop type definition
| text={translate('common.apply')} | ||
| pressOnEnter | ||
| variant={CONST.BUTTON_VARIANT.SUCCESS} | ||
| size={largeButton ? CONST.BUTTON_SIZE.LARGE : undefined} |
There was a problem hiding this comment.
Same as DateFilterContent 'size' comment
| text={translate('common.confirm')} | ||
| pressOnEnter | ||
| variant={CONST.BUTTON_VARIANT.SUCCESS} | ||
| size={largeButton ? CONST.BUTTON_SIZE.LARGE : undefined} |
There was a problem hiding this comment.
Same as DateFilterContent 'size' comment, if many cases like these occur we can extract new specific type
| onPress={() => onSelect(null)} | ||
| /> | ||
| > | ||
| <Button.Text style={[styles.alignSelfCenter, !isOffSelected ? styles.textSupporting : undefined]}>{translate('common.off')}</Button.Text> |
There was a problem hiding this comment.
!isOffSelected && styles.textSupporting, accordingly to all similar usages in the file
| sentryLabel={CONST.SENTRY_LABEL.WORKSPACE.RULES.SPEND_RULE_RESTRICTION_TYPE} | ||
| /> | ||
| > | ||
| <Button.Text style={[styles.alignSelfCenter, !isOffSelected ? styles.textSupporting : undefined]}>{translate('common.off')}</Button.Text> |
There was a problem hiding this comment.
Same as in SpendRuleRestrictionTypeToggle.tsx
| sentryLabel={CONST.SENTRY_LABEL.WORKSPACE.RULES.SPEND_RULE_RESTRICTION_TYPE} | ||
| /> | ||
| > | ||
| <Button.Text style={[styles.alignSelfCenter, !isAllowSelected ? styles.textSupporting : undefined]}>{translate('workspace.rules.spendRules.allow')}</Button.Text> |
…mButton's Button.Text
|
@ZhenjaHorbach Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 656888a757
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| variant={item.isPrimary ? CONST.BUTTON_VARIANT.SUCCESS : undefined} | ||
| {...props.buttonProps} | ||
| > | ||
| <Button.Text {...props.textProps}>{item.translationKey ? translate(item.translationKey) : item.text}</Button.Text> |
There was a problem hiding this comment.
When callers still pass translation keys in text and set shouldUseLocalization (for example join requests, fraud alerts, mention whispers, and the track-expense buttons in ChatActionableButtons), this branch no longer translates them because it only calls translate() for item.translationKey. Those existing callers were not migrated, so affected buttons will display raw keys such as actionableMentionTrackExpense.submit or common.yes instead of user-facing labels. Either keep honoring shouldUseLocalization here or update all callers to use translationKey.
Useful? React with 👍 / 👎.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| selectedDateModifier: SearchDateModifier | null; | ||
| hasFeed: boolean; | ||
| largeButton?: boolean; | ||
| size?: Exclude<ValueOf<typeof CONST.BUTTON_SIZE>, 'small'>; |
There was a problem hiding this comment.
❌ CONSISTENCY-2 (docs)
The Exclude<ValueOf<typeof CONST.BUTTON_SIZE>, 'small'> type hardcodes the string literal 'small'. This value is already defined as the named constant CONST.BUTTON_SIZE.SMALL, so duplicating the raw literal couples this type to the const's value: if CONST.BUTTON_SIZE.SMALL ever changed, the Exclude would silently stop excluding the intended member.
Reference the constant instead:
size?: Exclude<ValueOf<typeof CONST.BUTTON_SIZE>, typeof CONST.BUTTON_SIZE.SMALL>;Reviewed at: 656888a | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| values: Partial<SearchAdvancedFiltersForm> | undefined; | ||
| selectedField: PolicyReportField | null; | ||
| largeButton?: boolean; | ||
| size?: Exclude<ValueOf<typeof CONST.BUTTON_SIZE>, 'small'>; |
There was a problem hiding this comment.
❌ CONSISTENCY-2 (docs)
The Exclude<ValueOf<typeof CONST.BUTTON_SIZE>, 'small'> type hardcodes the string literal 'small' rather than the existing named constant CONST.BUTTON_SIZE.SMALL. Using the raw literal couples the type to the const value and will drift silently if that value changes.
Reference the constant instead:
size?: Exclude<ValueOf<typeof CONST.BUTTON_SIZE>, typeof CONST.BUTTON_SIZE.SMALL>;Reviewed at: 656888a | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| value: string | undefined; | ||
| isNegated: boolean; | ||
| largeButton?: boolean; | ||
| size?: Exclude<ValueOf<typeof CONST.BUTTON_SIZE>, 'small'>; |
There was a problem hiding this comment.
❌ CONSISTENCY-2 (docs)
The Exclude<ValueOf<typeof CONST.BUTTON_SIZE>, 'small'> type hardcodes the string literal 'small' instead of the already-imported named constant CONST.BUTTON_SIZE.SMALL. Referencing the literal duplicates the const value and will not stay in sync if the constant changes.
Reference the constant instead:
size?: Exclude<ValueOf<typeof CONST.BUTTON_SIZE>, typeof CONST.BUTTON_SIZE.SMALL>;Reviewed at: 656888a | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
JmillsExpensify
left a comment
JmillsExpensify
left a comment
There was a problem hiding this comment.
No product review required.
4 participants
Explanation of Change
This is batch 5 (PR5 of 9) of an ongoing effort to migrate every direct
<Button>usage (import Button from '@components/Button') over to the new composedButtonComposed, so the oldButtoncomponent can eventually be deprecated. This batch covers the "text-only · styled · search+reports" shape: 20 files / 29 button instances.The migration issue lists 23 files, but three were deleted in
ffc0c2baea7("Cleanup of screens related to old search filter routes"), taking 4 usages with them:src/components/Search/SearchFiltersAmountBase.tsxsrc/pages/Search/SearchAdvancedFiltersPage/SearchFiltersReportFieldPage/index.tsxsrc/pages/Search/SearchAdvancedFiltersPage/SearchFiltersWithdrawalTypePage.tsxNotes for review:
extraSmalldeleted from the exportedActionCellProps(src/components/Search/SearchList/ListItem/ActionCell/index.tsx, also dropped in.../ActionCell/DeferredActionCell.tsx). Already dead — Migrate ComposedButton to ButtonWithDropdownMenu #93789 removed it fromPayActionCell's props and deleted the only caller that passed it in (ReportListItemHeader, which passed!isLargeScreenWidthfrom inside anisLargeScreenWidth &&block, so alwaysfalse). Both call sites weresmall={!extraSmall}, so they rendered SMALL before and render SMALL now.src/components/Search/SearchList/ListItem/ActionCell/index.tsx— the "View" link, now aLinkButtonsrc/components/Search/SearchList/ListItem/TransactionGroupListExpanded.tsx— "Show more", also aLinkButtonsrc/components/ReportActionItem/TransactionPreview/TransactionPreviewContent.tsx— "Keep this one"src/components/Search/FilterComponents/DateFilterBase.tsx— "Reset"src/components/Search/SearchFilterPageFooterButtons.tsx— both "Reset" and "Save"Fixed Issues
$ #95174
PROPOSAL: #83762 (comment)
Tests
For every migrated button, verify that behaviour is unchanged from before the migration:
success= green,danger= red, default = grey.The migrated buttons fall into a few categories by the styling or behaviour applied to them. Steps for one
representative of each:
Segmented control (Off / Allow / Block)
Preconditions
Test steps
/workspaces/<workspace ID>/rules/spend-rules/new.Expected behavior
The three buttons look like one control — equal heights, edges touching, a single border — matching production.
The selected one is filled (green for Allow, red for Block, plain for Off); the other two are
transparent with grey labels.
Search row action button (color, offline, and inert-but-not-dimmed)
Preconditions
Test steps
Expected behavior
online it looks normal and works again immediately, with no stuck state.
Button with a loading spinner
Preconditions
At least two expenses not on an approved or paid report.
Test steps
src/pages/Search/SearchEditMultiple/SearchEditMultiplePage.tsx, change theisSavingline touseState(true), reach the screen again, then revert.Expected behavior
The label is replaced by a centred spinner — hidden, not pushed aside — and the button can't be clicked while it
shows. Then the panel closes.
Button whose label styling comes from the screen using it
Preconditions
None.
Test steps
Expected behavior
Buttons stacked vertically, reading Submit it to someone and Nothing for now — grey, single-line, sized
as in production. (Categorize it / Share it with my accountant may also appear, and there's no green
button here — both are correct.)
For the green variant: in a workspace room, mention a room that doesn't exist (e.g.
#does-not-exist-123). Thereply offers a green Yes and a grey No.
Offline tests
QA Steps
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.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
button_variants_small_android.mp4
Android: mWeb Chrome
button_variants_small_android_web.mp4
iOS: Native
button_variants_small_ios.mp4
iOS: mWeb Safari
button_variants_small_ios_web.mp4
MacOS: Chrome / Safari
button_variants_small_web.mp4