-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
feat: Add report field picker components #34157
Conversation
@allroundexperts Are you able to resolve the failing TS checks? |
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid-native-2024-01-22_16.08.27.mp4Android: mWeb Chromeandroid-chrome-2024-01-22_16.12.37.mp4iOS: Nativeios-native-2024-01-22_15.41.13.mp4iOS: mWeb Safariios-safari-2024-01-22_15.54.36.mp4MacOS: Chrome / Safaridesktop-chrome-2024-01-22_15.56.04.mp4MacOS: Desktopdesktop-app-2024-01-22_15.57.47.mp4 |
@jjcoffee All done! |
@allroundexperts I get a bunch of these console warnings when opening the dropdown field: |
Fixed! |
7ad9d34
to
399502d
Compare
Hi @jjcoffee! Thanks! |
Sorry yes, on it now! |
@allroundexperts Shouldn't the |
It should, I'll investigate why it isn't. But please continue your review and let me know if there's anything else that you find. |
@jjcoffee Fixed. |
@allroundexperts I think it needs a new section for displaying (and highlighting) the selected item at the very top, per the mockup (see also the Category selector for money requests): I'm also seeing that searching doesn't highlight the top result, which might be related. Also, maybe you missed it but I mentioned that the |
Recents would be handled in an upcoming PR. But, I'll look into the other issues. |
@jjcoffee I think we can take care of #34157 (comment) as a follow up. There are couple of PRs that are depending on this one so it would be good if we can merge this. |
Will you be able to complete the review today? |
Co-authored-by: Joel Davies <joeld.dev@gmail.com>
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.
LGTM! The only outstanding issue is with the dropdown, which @allroundexperts has proposed to deal with in a follow up PR. The dropdown search should also highlight the topmost matching search result, I think (which is probably more or less the same fix).
@thienlnam looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
Perf tests known to be flakey |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
@allroundexperts could you help us with step 3:
|
🚀 Deployed to staging by https://github.com/thienlnam in version: 1.4.30-0 🚀
|
@kavimuru All QA accounts should have all betas enabled I believe |
🚀 Deployed to production by https://github.com/thienlnam in version: 1.4.30-1 🚀
|
1 similar comment
🚀 Deployed to production by https://github.com/thienlnam in version: 1.4.30-1 🚀
|
return [ | ||
{ | ||
title: translate('common.recents'), | ||
shouldShow: true, |
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.
We should have shown this only if recent options were greater than 0. This later caused #35832
Details
This PR adds report field picker components.
Note: Saving those fields is beyond the scope of this ticket. It will be handled in an upcoming ticket.
Fixed Issues
$ #32763
PROPOSAL: #32763
Tests
policyID
with thepolicyID
of thepolicy
the opened report is attached to:canUseReportFields
beta inPermissions.ts
file.Offline tests
N/A
QA Steps
policyID
with thepolicyID
of thepolicy
the opened report is attached to:canUseReportFields
beta inPermissions.ts
file.PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Screen.Recording.2024-01-09.at.8.23.41.PM.mov
Android: mWeb Chrome
Screen.Recording.2024-01-09.at.8.22.36.PM.mov
iOS: Native
Screen.Recording.2024-01-09.at.8.20.44.PM.mov
iOS: mWeb Safari
Screen.Recording.2024-01-09.at.8.16.42.PM.mov
MacOS: Chrome / Safari
Screen.Recording.2024-01-09.at.8.04.04.PM.mov
MacOS: Desktop
Screen.Recording.2024-01-09.at.8.07.09.PM.mov