-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Fix/56339 fetch available cards to assign #57003
base: main
Are you sure you want to change the base?
Fix/56339 fetch available cards to assign #57003
Conversation
@DylanDylann 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] |
@koko57 I think we need to call API when clicking the next button instead of clicking the feed Screen.Recording.2025-02-19.at.17.01.10.mov |
@DylanDylann no, because we need the info of the cardList for skipping the step logic |
@koko57 Oh, Good catch |
2261d7a
to
6e7c0b5
Compare
6e7c0b5
to
2cb18f1
Compare
The change works well. Waiting for @mountiny to provide a new API |
OpenAssignFeedCardPage is ready |
@mountiny @DylanDylann api call replaced |
I approved by mistake |
Screen.Recording.2025-02-20.at.12.10.04.mp4added the loader |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2025-02-21.at.14.28.00.movAndroid: mWeb ChromeScreen.Recording.2025-02-21.at.14.02.46.moviOS: NativeScreen.Recording.2025-02-21.at.14.07.19.moviOS: mWeb SafariScreen.Recording.2025-02-21.at.14.01.26.movMacOS: Chrome / SafariScreen.Recording.2025-02-21.at.13.58.09.movMacOS: DesktopScreen.Recording.2025-02-21.at.14.05.13.mov |
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 change looks fine to me. Just an improvement, we can prevent from calling API if the data already be fetched
Screen.Recording.2025-02-21.at.14.07.19.mov
ok, I can do that in this PR |
@koko57 Great |
@DylanDylann TBH not sure if we can do this here - we set selected feed in the state asynchronously and until we set it we have a different value for
so until we have a new list for a newly selected feed we cannot check if to has the cardList object or not and say for sure if we should fire the API call or not. I could possibly fire the call to the API in a useEffect with selectedFeed dependency, but I'm afraid that there could be some race conditions when getting new selectedFeed, list and fetching the new data. I wonder if there is also a possibility that the user will click next before the API call is sent, what could result in no cards to be assigned on thelist. |
I think it shouldn't block this PR, we can discuss it later |
Anyways, I have a solution. We can create an util function to get WORKSPACE_CARDS_LIST and in this function, we will use Onyx.connect to get Onyx data |
@DylanDylann great, thanks! I haven't thought of that - should I proceed with this in this PR? |
@DylanDylann can you retest please? |
Explanation of Change
Fixed Issues
$ #56339
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
same as above
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.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 and/or tagged@Expensify/design
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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop