Conversation
|
|
|
This one surely looks like the PR I was working on back then - #72248 I can help as C+ here if needed. |
…s/@zfurtak/migrate-NewChatPage" This reverts commit 1f133a0.
melvin-bot
bot
requested review from
dukenv0307 and
heyjennahay
and removed request for
a team
|
@dukenv0307 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] |
|
How do we test this one? Do you have before & after screenshots so we know what to look out for? |
![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: d0ce3e68e8
ℹ️ 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".
| const updateAndScrollToFocusedIndex = (index: number, shouldScroll = true) => { | ||
| if (!shouldScroll) { | ||
| suppressNextFocusScrollRef.current = true; |
There was a problem hiding this comment.
The new updateAndScrollToFocusedIndex() implementation changes the second argument from the old “skip if a nonzero item is already focused” behavior to “should scroll”. Existing callers were not updated: SearchRouter still invokes updateAndScrollToFocusedIndex(1, true) from onHighlightFirstItem (src/components/Search/SearchRouter/SearchRouter.tsx:305,354) to avoid stealing focus after the user has arrowed to a later suggestion. With this change, typing another character while browsing autocomplete results snaps focus back to the first result on every update, so keyboard users can no longer keep a lower suggestion selected while refining the query.
Useful? React with 👍 / 👎.
| const [isReportInOnyx] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${item.reportID}`, { | ||
| selector: (report) => !!report, | ||
| }); |
There was a problem hiding this comment.
Unlike UserListItem (src/components/SelectionList/ListItem/UserListItem.tsx:55-56), this new component interpolates item.reportID directly into the Onyx key. Several option builders still produce reportID: '' for people without an existing chat, so those rows subscribe to ONYXKEYS.COLLECTION.REPORT instead of a single report. In the autocomplete and money-request pickers that means any report update rerenders every visible row, which will noticeably degrade list performance under normal report traffic.
Useful? React with 👍 / 👎.
|
hi @shawnborton as I mentioned here, I'm not sure how to test it. It's especially difficult because we don't have any list of places where it needs to be changed. What about running a test suite? The QAs could then attach screenshots from production and this PR, and we can discuss if the changes are correct. It's just a suggestion, though, wdyt? 😊 |
|
Can you try to do a better job of describing the changes? I don't know what this means:
Again, before and after screenshots would be super helpful, even if nothing changes. It at least helps us understand where we should be looking though. |
|
Should I look through all 130 usages of SelectionList in the app? That's quite a lot of cases but can do 😅 |
|
Heh, I think we might be speaking past each other. Can you give one simple example of what your PR is doing here? A quick before/after of one spot that your PR touched? |
|
Now I get it 😊 Sure thing! FIrst example is multi-choice selection list The second one is single-choice list |
@zfurtak you could checkout the test section of my original PR - #72248 (comment)
I essentially did that and that's why asked to be c+ here, as I have the most context here. |
|
@shawnborton this is reincarnation of - #66777 |
|
Lovely! I'll run some adhoc builds for easier testing too. Thanks! |
|
🚧 @shawnborton 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! 🧪🧪
|
|
Should we also have the single select (radio box) pattern on the subsequent page of Workflow > Add Approval? |
|
Missing here in Categories > Category > Settings: |
|
Overall this is feeling really good to me! |
Probably yes, but this is also one of those weird scenarios where we auto-proceed you so it's a bit weird. Still think we probably should though. |
|
Some seem to have a background color on selection and some don't. Why is this? I'd love to make that consistent. I prefer to have them not be there, but curious if @Expensify/design agrees or not.
|
|
Down to remove them and see how it feels. |
|
Let's remove it then. If we don't like it and wanna keep it, I kinda wanna see if we can use row-hover (ignoring the double hover effect) or something that is just more subtle to avoid the overall blocks of background on multi-select screens |
thank you @ChavdaSachin I copied the tests 👍 |
5 participants
Explanation of Change
In the PR I:
The idea is to enhanced consistency among lists with selection.
Important
This PR requires #84880 to be merged first
Fixed Issues
$ #74938
PROPOSAL:
Tests
same as qa tests
Offline tests
QA Steps
Single selection list
For each of the following pages verify:
Reports
Workspace
Overview
Members
Reports
Accounting - QBO
-Accounting - QBD
import > classes > displayed as
import > customers/projects > displayed as
export > preferred exporter
export > export date
export > export out-of-pocket expenses as > export as
export > export out-of-pocket expense as > account
export > export invoices to
export > export company card expenses as > export as
export > export company card expenses as > credit card account
Accounting - Xero
Accounting - NetSuite
Accounting - Sage Intacct
Categories
Tags
tag > approver
Taxes
Workflows
Rules
Distance rates
Expensify cards
Company cards
Per diem
Account
Profile
Subscription
Wallet
Preferences
Security
Troubleshoot
FAB
Create expense
Create report > choose workspace
Start chat
Chat
-Tracked expense
submit to someone > choose recipient
Workspace expense
create task > assignee
task > assignee
room chat
Onboarding
Multi selection
For each of the following pages verify:
Reports
Filters > type = expense
Filters > type = chat
Filters > type = task
Workspace
Overview
Members
Workflows
FAB
Onboarding
New chat item
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand 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