-
Notifications
You must be signed in to change notification settings - Fork 3.5k
fix add approver cut off #78607
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.
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
base: main
Are you sure you want to change the base?
fix add approver cut off #78607
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
The original plan was to migrate the App/contributingGuides/SELECTION_LIST.md Line 11 in ae4cd7c
Additionally, as per the migration plan outlined in this comment, the intention is to first handle non-section use cases and later address section-based ones, potentially by optimizing SelectionList or introducing a new specialized component for multi-section scenarios. Given this, keeping the |
|
Agreed. Thanks for the explanation. I also recently noticed while reviewing PRs we should use |
| initiallyFocusedOptionKey={selectedOptionKey ?? undefined} | ||
| addBottomSafeAreaPadding={addBottomSafeAreaPadding} | ||
| contentContainerStyle={contentContainerStyle} | ||
| listItemTitleStyles={styles.mnw100} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use the same approach as used in new SelectionList? Or is mnw100 already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new SelectionList doesn't use a special approach, it uses FlashList instead of SectionList, which avoids the truncation issue. Since SelectionListWithSections uses SectionList, adding mnw100 via listItemTitleStyles is the correct fix for this component.
Explanation of Change
Fixed Issues
$ #77357
PROPOSAL: #77357 (comment)
Tests
Precondition:
Android 16 only.
Device font size is minimum.
Add approveris shown instead of justAddOffline tests
Same as above
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Precondition:
Android 16 only.
Device font size is minimum.
Add approveris shown instead of justAddPR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, 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.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