-
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
[Search v1] Create Search widget #40903
[Search v1] Create Search widget #40903
Conversation
5e147de
to
6563edd
Compare
747ea2b
to
07c73f2
Compare
07c73f2
to
bb4195b
Compare
@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] |
Ready to review but I have a TS error which I do not yet understand, I will be fixing it tomorrow |
tooltip?: string; | ||
useEffect(() => { | ||
SearchActions.search(query); | ||
}, [query]); |
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.
Hmm I found it to be a bit weird that refreshing the page doesn't trigger a call to search, but that's something we could iron out later in a follow up if needed.
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.
I believe the search()
gets called, but its the implementation of isNetworkOffline
thats breaking it.
I did some logging and in the beginning the NETWORK sets isOffline
to true for a split second, and that is when the useEffect triggers. Then a bit later isOffline
is correctly set to false
but by then effect has already run and no search call was made.
Not sure how to fix this, need some guidance from you. Is that the intended behavior of ONYXKEYS.NETWORK
?
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.
I don't think that's a blocker for now. Let's move on and address this as a follow up.
@luacmartins thanks for the comments, I implemented almost all the changes you requested. There is one thing which I would like to do in a separate PR, and that is the proper implementation of However the next issue to do is actually the items list + TransactionLIstItem component, and those will have to be bound via interface, because thats is what So I would prefer to keep this PR as is and fix this in another PR, where I would have to do a common interface anyways. It will be easier to do it then, right now it will require a bit of TS-hacking to achieve this |
That makes sense, let's address the proper implementation of those methods in a follow up then! |
@dukenv0307 could you please prioritize reimbursing this PR when you're online? Thank you! |
I'm reviewing... |
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 and tests well with the API
return ( | ||
<SearchFiltersNarrow | ||
filterItems={filterItems} | ||
activeItemLabel={activeItemLabel} | ||
activeItemLabel={String(query)} |
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.
activeItemLabel={String(query)} | |
activeItemLabel={query} |
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.
Let's not block on this. We can address this in a follow up @Kicu @WojtekBoman
@Kicu @luacmartins just have one minor comment. Otherwise it looks good |
Reviewer Checklist
Screenshots/Videos |
Nice! Let's not block on that comment. I can post the screenshots since I have access to the API internally. Unless @dukenv0307 has them already :) |
@luacmartins I'm doing the screenshots and videos. But I just use the mock data provided by @Kicu |
Sounds good! Thank you! |
Tests well, @luacmartins we're good to merge |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/luacmartins in version: 1.4.67-0 🚀
|
🚀 Deployed to production by https://github.com/francoisl in version: 1.4.67-7 🚀
|
1 similar comment
🚀 Deployed to production by https://github.com/francoisl in version: 1.4.67-7 🚀
|
Details
Fixed Issues
$ #39879
PROPOSAL:
Tests
fake data code snippet
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.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
empty
with temp data
MacOS: Desktop