Fix: Replace InviteMemberListItem with SingleSelectWithAvatarListItem on DomainAddAdminPage#81815
Conversation
…omainAddAdminPage
melvin-bot
Bot
requested review from
Ollyws and
heyjennahay
and removed request for
a team
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| return ( | ||
| <Checkbox | ||
| shouldSelectOnPressEnter | ||
| containerBorderRadius={999} |
There was a problem hiding this comment.
❌ CONSISTENCY-2 (docs)
The value 999 is a magic number without documentation. This border radius value should be defined as a named constant to improve code readability and maintainability.
Suggested fix:
const CIRCULAR_BORDER_RADIUS = 999; // Large value to create circular border
const radioCheckboxComponent = useCallback(() => {
return (
<Checkbox
shouldSelectOnPressEnter
containerBorderRadius={CIRCULAR_BORDER_RADIUS}
accessibilityLabel="SingleSelectListItem"
isChecked={isSelected}
onPress={() => onSelectRow(item)}
/>
);
}, [isSelected, item, onSelectRow]);Or better yet, if this value is used elsewhere in the codebase, it should be defined in a shared constants file.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Agreed, can you please add a style constant somewhere for circular/full border radius?
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@shawnborton Does the 
|
|
Hmm I would think each of these rows would be 64px tall (12px vertical padding + 40px avatar = 64px), basically can we reuse our standard row pattern we have elsewhere for displaying avatar + names? |
@shawnborton Perhaps we need to use existing
Yeah that does make sense, i have looked into this , all the PS: Will |
|
Okay keep us posted! |
|
Cool .... |
|
@shawnborton Update: Added 
Here's the 
I think this looks correct to me, wdyt? |
|
I do think that looks correct as well, thanks! Let's see if the others have thoughts too. |
|
I think i have played with this and all the |
|
@Ollyws Bump for the review here. Thanks! |
|
Looks good to me 👍 |
|
@PiyushChandra17 The lint and TS checks are failing. |
|
@Ollyws Yep, this has picked up |
|
@Ollyws Resolved the failing Lint && TS checks ✅ |
|
@Ollyws Bump |
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp
Android: mWeb Chrome
iOS: HybridApp
iOS: mWeb Safari
MacOS: Chrome / Safari
|
Ollyws
left a comment
Ollyws
left a comment
There was a problem hiding this comment.
Melvin's comment https://github.com/Expensify/App/pull/81815/changes#r2780590122 does make sense, but other than that LGTM.
chuckdries
left a comment
chuckdries
left a comment
There was a problem hiding this comment.
Looks great! Just a small nit
| return ( | ||
| <Checkbox | ||
| shouldSelectOnPressEnter | ||
| containerBorderRadius={999} |
There was a problem hiding this comment.
Agreed, can you please add a style constant somewhere for circular/full border radius?
|
Hm, looks like we're failing prettier and lint checks. Can you merge main and try to run the checks locally? |
|
@chuckdries Yeah seems like the prettier check is failing even after |
|
@PiyushChandra17 I just realized I was misinterpreting the stuff going on in the open source channel. It does appear to be broken in main in CI (but not on my laptop????). Looking into it as well |
|
Agree, i think it's broken in the main CI .. Perhaps let's wait to get this fixed .. And merge latest main and get this going ✅ |
|
@chuckdries Ran |
|
🚧 @chuckdries has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/chuckdries in version: 9.3.26-0 🚀
|
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.3.26-8 🚀
|
6 participants
Explanation of Change
Replace
InviteMemberListItemwithSingleSelectWithAvatarListItemonDomainAddAdminPageFixed Issues
$ #80353
PROPOSAL: #80353(Comment)
Tests
DomainAddAdminPage> Verify the selected contact now has newly introducedSingle Select List Itemstyles withAvatarsand hovering over the item, the row gets aBackground HighlightOffline tests
Same as tests
QA Steps
Same as tests
PR 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